Integrate Water sources from a separate repository #288
Conversation
ftynse
force-pushed
the
users/ftynse/vendor-water-2
branch
2 times, most recently
from
46033b0 to
6294014
Compare
|
This is the minimal amount of code that preserves Wave tests. The rest should be landed separately. |
harsh-nod
left a comment
harsh-nod
left a comment
There was a problem hiding this comment.
overall looks great! I noticed this has nothing about the Wave Dialect but I imagine that is coming in the next few PRs?
| // | ||
| // Licensed under the Apache License v2.0 with LLVM Exceptions. | ||
| // See https://llvm.org/LICENSE.txt for license information. | ||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception |
There was a problem hiding this comment.
Could you add some documentation on these 2 passes similar to what we have in the read the docs page?
There was a problem hiding this comment.
This documentation is available in Passes.td. At some later point, we may auto-generate a web version of that, but these are implementation details that need not be exposed to the user.
There was a problem hiding this comment.
I agree that these are details that most kernel authors won't need (at least initially), but would be great to have a description of the pass with an example (see https://wave-lang.readthedocs.io/en/latest/wave/shared_memory.html). But this is fine for now and we can iterate on it.
There was a problem hiding this comment.
Examples in the documentation get outdated within weeks and is rarely updated when the logic changes. A somewhat better solution is to add a pointer to a test exercising the pass, those at least have to be modified when pass behavior or IR syntax changes. They can be moved/removed, but it happens less frequently.
This allows to remove the dependency at build/CI time and co-develop the dialect inside Wave tree. Both source bases are licensed under Apache 2 with LLVM exceptions, so there is no additional work needed there. Co-authored-by: Martin Lücke <martin.luecke@amd.com> Co-authored-by: Ivan Butygin <ivan.butygin@gmail.com> Co-authored-by: Tim Gymnich <tim.gymnich@amd.com> Signed-off-by: Alex Zinenko <git@ozinenko.com>
ftynse
force-pushed
the
users/ftynse/vendor-water-2
branch
from
6294014 to
d4efc27
Compare
As the comment above says, this is the strict minimal amount allowing to remove the dependency. I am not particularly keen on squashing even this amount of history, but this is a trade-off with relanding each commit separately. |
harsh-nod
left a comment
harsh-nod
left a comment
There was a problem hiding this comment.
thanks for integrating this!
This allows to remove the dependency at build/CI time and co-develop the dialect inside Wave tree. Both source bases are licensed under Apache 2 with LLVM exceptions, so there is no additional work needed there. Co-authored-by: Martin Lücke <martin.luecke@amd.com> Co-authored-by: Ivan Butygin <ivan.butygin@gmail.com> Co-authored-by: Tim Gymnich <tim.gymnich@amd.com> Signed-off-by: Alex Zinenko <git@ozinenko.com>
This allows to remove the dependency at build/CI time and co-develop the
dialect inside Wave tree.
Both source bases are licensed under Apache 2 with LLVM exceptions, so
there is no additional work needed there.
Co-authored-by: Martin Lücke martin.luecke@amd.com
Co-authored-by: Ivan Butygin ivan.butygin@gmail.com