Skip to content

precompute generates invalid code when I give it differing index variable names #431

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Infinoid opened this issue Mar 26, 2021 · 5 comments
Assignees
Labels
bug Indicates an unexpected problem or unintended behavior

Comments

@Infinoid
Copy link
Contributor

This works: taco 'A(i) = B(i)*1' -s='precompute(B(i)*1,i,i)'
This breaks: taco 'A(i) = B(i)*1' -s='precompute(B(i)*1,i,j)'

These two commands generate quite different output. The output of the first one appears correct at first glance; the output of the second one has several issues:

  • somehow, the size of the workspace stopped being equal to the input size, and is now hardcoded to 42
  • the main copy loop now loops over the workspace var, not the input var, and is more complicated as a result
  • the output loop now refers to two index variables that are never defined (iA and iworkspace)

If you then add -f=B:s so that the B tensor is sparse, the output of the first case still appears correct, but the second one now has an additional problem:

  • the main copy loop now writes to workspace positionj. j is initialized to i, but never updated from one loop iteration to the next

The full outputs are here. This was using a Debug build of taco with no features (openmp, cuda, or python).

There's some overlap between this and comments in issue #347. But I think the isLowerable failure is a separate issue.

@rohany
Copy link
Contributor

rohany commented Mar 26, 2021

I don't know much about this, but it looks like the two commands you have written are the same.

@Infinoid
Copy link
Contributor Author

The difference is in the precompute() scheduling directive, the ,i or ,j at the end.

Giving it the same IndexVar name twice works. Giving two different names breaks.

@rohany
Copy link
Contributor

rohany commented Mar 27, 2021

Ah my bad, I stared at this for a while and couldn't tell that the i and j were different.

@weiya711 weiya711 added the bug Indicates an unexpected problem or unintended behavior label Apr 30, 2021
@weiya711
Copy link
Contributor

This is a known bug and happens whenever i and iw differ when calling precompute(expr, i, iw) differ. We are working on fixing this code generation issue now. @Infinoid you are correct that the isLowerable failure from #347 is different since the i and iw differing bug should still generate (albeit incorrect) code without any assertion errors

@Infinoid
Copy link
Contributor Author

Looks like #459 fixed this. Thanks @nirvikbaruah!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

No branches or pull requests

3 participants