Skip to content

Conversation

mikeurbach
Copy link
Contributor

This is not a totally routine bump.

Python support is changing in preparation for a switch to nanobind:

https://discourse.llvm.org/t/psa-python-binding-dependencies-changing/83376

There were also some more DialectConversion changes:

llvm/llvm-project#116470

@mikeurbach
Copy link
Contributor Author

@mortbopet could you help take a look at the dialect conversion differences? I believe I've updated all of the patterns that started throwing errors indicating they needed to be 1:N patterns, but after I've updated those similarly to how it was done upstream, the tests still aren't passing. I think you're more familiar with these conversion patterns (FlattenIO and pruning 0 width). It might be that we need to start using replaceOpWithMultiple in some cases? I'm not really sure what else should be changed.

@mikeurbach
Copy link
Contributor Author

@fabianschuiki there's also a failure related to SMT. Would you happen to know anyone else who could take a look there?

@mikeurbach
Copy link
Contributor Author

Looking at the original PR and the subsequent update, I guess what we need here is different overloads, not getting rid of the old overload. I'll see if changing that fixes these.

`InstanceOpConversion`'s 1:1 in FlattenIO generalizes to the 1:N pattern, so we can drop the 1:1 pattern.

Secondly, it seems like the biggest violating thing here is the fact that we were missing a materialization for exploding struct outputs of `hw.module.extern`... This was actually a pretty glaring mistake in the existing IR, which had a check for invalid IR - woops!

Combined with folders, this also seems to have removed some (not all!) of the redundant struct_create/struct_explode patterns in the output.
@mortbopet
Copy link
Contributor

@mikeurbach thank you for the ping.

flatten-op should now be fixed; InstanceOpConversion's 1:1 in FlattenIO generalizes to the 1:N pattern, so we can drop the 1:1 pattern.

Secondly, it seems like the biggest violating thing here is the fact that we were missing a materialization for exploding struct outputs of hw.module.extern... This was actually a pretty glaring mistake in the existing IR, which had a check for invalid IR - woops!

Combined with folders, this also seems to have removed some (not all! - probably something to look into) of the redundant struct_create/struct_explode patterns in the output.

with the 1:N operand adaptor, we no longer have to manually filter i0 operands inside a conversion pattern. Instead, this information is already implicitly available via the adaptor (i.e. that an operand was removed via. materialization). This also implies that 1:N patterns need to handle the case where the `OneToNOpAdaptor` is empty.

In general, it feels like one would _very_ rarely have to use both the `OneToNOpAdaptor` and `OpAdaptor` overloads at the same time - this should be reserved for when there is truly a difference between 1:1 and 1:N patterns. However, in practice - as this little fixing excercise has demonstrated - most patterns where `OneToNOpAdaptor` is relevant, are generalized 1:N patterns, and doesn't need a specific `1:1` overload.
@mortbopet
Copy link
Contributor

And 9d25ea9 should fix pruning,

With the 1:N operand adaptor, we no longer have to manually filter i0 operands inside a conversion pattern. Instead, this information is already implicitly available via the adaptor (i.e. that an operand was removed via. materialization). This also implies that 1:N patterns need to handle the case where the OneToNOpAdaptor is empty.

In general, it feels like one would very rarely have to use both the OneToNOpAdaptor and OpAdaptor overloads at the same time - this should be reserved for when there is truly a difference between 1:1 and 1:N patterns. However, in practice - as this little fixing excercise has demonstrated - most patterns where OneToNOpAdaptor is relevant, are generalized 1:N patterns, and don't need a specific 1:1 overload.

@maerhart
Copy link
Member

@fabianschuiki there's also a failure related to SMT. Would you happen to know anyone else who could take a look there?

The SMT failure is coming from llvm/llvm-project@2a30bfc
You can just copy-paste the new error message

@mikeurbach
Copy link
Contributor Author

@maerhart thanks for digging that up, I just updated that expected error message.

@mortbopet thanks for digging into this, actually taking the time to understand the implications of 1:N conversions, and updating our rewrites accordingly. That all makes sense to me.

I think this should pass CI now, and I'll plan to merge it today.

@mikeurbach mikeurbach marked this pull request as ready for review December 13, 2024 16:25
@mikeurbach
Copy link
Contributor Author

Hmm the Python tests are failing...

The lib is built for python 3.8: https://github.com/llvm/circt/actions/runs/12318793396/job/34384323746?pr=7976#step:8:118

But the tests are running with python 3.9: https://github.com/llvm/circt/actions/runs/12318793396/job/34384323746?pr=7976#step:8:133

Not sure yet why this is happening... I thought the only material change was switching to a docker image with nanobind.

@mikeurbach
Copy link
Contributor Author

Something is still finding Python 3.8 here while the rest of the CMake stuff is finding Python 3.9 https://github.com/llvm/circt/actions/runs/12322664291/job/34396701183#step:5:329...

@mikeurbach
Copy link
Contributor Author

alright, this is working. going to get this in now.

@mikeurbach mikeurbach merged commit 387291f into main Dec 13, 2024
4 checks passed
@mikeurbach mikeurbach deleted the mikeurbach/bump-llvm branch December 13, 2024 21:29
@teqdruid
Copy link
Contributor

Thanks for all the leg work @mikeurbach!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants