[llvm] [mlir] [MLIR] Generalize expand_shape to take shape as explicit input (PR #90040)

Gaurav Shukla via llvm-commits llvm-commits at lists.llvm.org
Fri Apr 26 10:21:34 PDT 2024


Shukla-Gaurav wrote:

> > @ftynse unless this arith -> utils dependency breaks builds (I dont think it does, cause there is no cyclic dependency afaics), i think it might be better to land this and then remove the dependency as a quick follow up.
> 
> I don't see a cyclic dependency immediately. This is a bit of a bikeshedding discussion, one can simply move a file under a new `ShapedTypeArithUtils` and resolve the surface concern: `DialectUtils` suggests that it's something cheap and useful for all dialects, but it comes with a dependency on Arith (and UB, etc.). There's a deeper concern: we don't have a dialect-agnostic way to create constants, so anything that creates constants will have to depend on Arith or similar. I certainly won't block this on resolving that concern.
> 
> > This is fairly big change, and more things might get added that will have to be resolved. I am inclined to take the snap shot as is, but fix the dependency issue quickly after.
> 
> If the issue can be fixed quickly, why not quickly add the solution to the PR? :)
> 
> My worry here is by allowing this dependency temporarily, more uses will creep in, making it harder to remove as the time goes. I have made a similar comment this week on another PR...
> 
> How about we ask a third opinion, @joker-eph ?

The utility has been moved to Arith/Utils, so no specific dialect dependency on the utils dialect. Let me know if Arith/Utils is the right place to put the utility `inferExpandShapeOutputShape`?

https://github.com/llvm/llvm-project/pull/90040


More information about the llvm-commits mailing list