Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Integer and FP types are not arith types, they are builtin. I don't see a reason why we should add a dependency on UB dialect here. Feels like a separate UBToLLVM would offer a cleaner layering.
Integer and FP types are not arith types, they are builtin. I don't see a reason why we should add a dependency on UB dialect here. Feels like a separate UBToLLVM would offer a cleaner layering.
Most likely we will have this dependency anyway (e.g. fold arith.div by 0 to poison) and it will force all arith users to include UBToLLVM into theirs pipeline even if they are not using UB dialect explicitly.
Alternatively, I can add a separate UBToLLVM conversion, but implicitly include it into ArithToLLVM.
Why would it need to be implicitly included in the conversion? We have a -test-lower-to-llvm pipeline that can be augmented, and downstreams are encouraged to set up their own pipelines or pattern mixes.
Why would it need to be implicitly included in the conversion?
If users are using populateArithToLLVM and we introduce poison based arith folding, it will break their code as they now have to add populateUBToLLVM even if they are not using UB dialect directly.
If users are using populateArithToLLVM and we introduce poison based arith folding, it will break their code as they now have to add populateUBToLLVM even if they are not using UB dialect directly.
MLIR development policy https://mlir.llvm.org/getting_started/DeveloperGuide/#breaking-changes allows us to land breaking changes. Having a sort of transitionary period and posting a PSA on the forums with transition guidelines are highly encouraged, but I am very uncomfortable with the idea of introducing wrong layering because it may break downstreams. If this were to become the norm, we would be essentially stuck with the current layering forever. Adding a new dialect is expected to affect some lowerings, I don't see why UB dialect would be any different.
I think, the problem is more general here, as at some point populateArithToLLVM may become just unusable without populateUBToLLVM as it will be very hard for users not to trigger some poison-related folding accidentally. We can clarify this dependency via docs, but it is a quite weak guarantee.
How so? Folding may be indeed triggered as part of the conversion, and it would produce an operation from the UB dialect. This is only a problem when a user has requested a full conversion and has not declared ops from the UB dialect as legal (or added the patterns to convert them). I agree that it is not the ideal situation as they may expect ArithToLLVM patterns to include patterns producing only LLVM operations, but sounds like something that documentation should clarify. We also have precedent with conversions producing a set of dialects, e.g. SCFToCF produces Arith, so it isn't entirely new either. On the opposite, if ArithToLLVM starts to also convert preexisting ops from the UB dialect, _that_ would be surprising and doesn't seem to have precedent upstream. Furthermore, I would expect that other patterns or folders beyond Arith may be producing ops from UB, so it would actually make more sense to have all UB converted at once.
I was thinking of keeping poison-producing patterns in a separate populate function, at least initially. These would generally be considered more aggressive and surprising, and separating them out would give us more time to flesh things out. Then we can have a separate conversion passes to handle them when we lower to llvm/spirv, like @ftynse suggests.
I was thinking of keeping poison-producing patterns in a separate populate function, at least initially.
Ok, this will work, but it makes most of PoisonAttr-related design discussion from discourse thread useless, as main usecase of attr is op folders.
I'm not saying we shouldn't do it in folders, but as soon as we start introducing poison, everything else down the lowering chain needs to handle it. What I propose i to initially have poison-producing rewrite patterns, plumb everything through, and then consider doing more folds.
There are few things that we haven't solved around the populate.... method:
- The number of dialects generated isn't documented or better queryable: that means that things may break even in a way that isn't surfaced in tests (triggered by a combination of folding is an example...).
- The populateArithToLLVM names are misleading as well, but I don't have a great suggestion :)
We should have a programmatic way of using these APIs more safely.
I would want a "PatternConfigurator" helper where the user could configure some source dialects and destination dialect and it'll take care of everything.
It's tricky because it'd require a lot of meta-programming magic...
There are few things that we haven't solved around the populate.... method:
Indeed. That's why I'd rather keep things close to the status quo, for which folks at least have developed some intuition.
I would want a "PatternConfigurator" helper where the user could configure some source dialects and destination dialect and it'll take care of everything.
It's tricky because it'd require a lot of meta-programming magic...
This would require some level of declarativeness so patterns or sets of patterns describe to the configurator what they can do. For the LLVM dialect specifically, I was pondering the idea of having a dialect interface that collects all patterns used for "trivial" lowering down to the LLVM dialect + list of dialects that can be produced by those patterns. We can then have a pass that queries the dialects present in the input and dialects that can be produced by lowerings to have a complete set of patterns, and then lowers in one sweep.