spcl / dace-vscode

Rich editor for SDFGs with included profiling and debugging, static analysis, and interactive optimization.
https://marketplace.visualstudio.com/items?itemName=phschaad.sdfv
BSD 3-Clause "New" or "Revised" License
19 stars 10 forks source link

dace extension display error for access patterns and stack distance #240

Open rohanrayan opened 1 year ago

rohanrayan commented 1 year ago

Describe the bug The extension doesn't animate matrix access for a simple use-case. Tested using

To Reproduce Steps to reproduce the behavior:

  1. Generate the SDFG using the attached ipynb
  2. Open the generated SDFG using the VS-code extension
  3. Select the entire SDFG and click on the "Inspect access patterns (local view)" tab
  4. In the pop-ups for M and N, enter 10 in both
  5. In the local view tab, try moving the slider and you will see that only one input matrix access gets highlighted.
  6. Both Access pattern and Reuse distance only show up for one matrix for some reason.

Expected behavior I expect both the input matrices to display the same access pattern in the extension.

dace version info dace v0.14.4 VS code extension v1.5.9

vs_code_bug_reproduce.zip

phschaad commented 1 year ago

Thank you for reporting the issue.

For completeness, I am including the source that generated the SDFG, and a picture of the SDFG here. Source program (simplified):

import dace

N = dace.symbol('N')
M = dace.symbol('M')

@dace.program(auto_optimize=True)
def madd(A: dace.float64[M, N], B: dace.float64[M, N]):
    for i in range(0, M):
        for j in range(0, N):
            A[i][j] += B[i][j]

SDFG:

image

Now, the reason the highlighting does not work as expected is because of this intermediate data container, __tmp0. Because of this, the accesses inferred from the computation do not happen to data container B, but rather the temporary container __tmp0. There are plans to change this behavior because it does not reflect the behavior when viewed on the entire map, as you were trying to do. I will bump the priority of that work item in response to this, since I'm glad to see the feature is being used ;-) I cannot promise that it will make it into the October patch of the extension though due to some other high priority items.

An additional note here, though, is that it seems like this is another manifestation of https://github.com/spcl/dace/issues/1382 - since it seems like the __tmp0 container should not be there to begin with. @alexnick83 ?

alexnick83 commented 1 year ago

There isn't any issue with the container being there in the first place but I would expect it to be removed by strict transformations (before auto-optimization making the loop parallel).