[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