[Mlir-commits] [mlir] [mlir][linalg] Fix UBSan division-by-zero in PackOp folding (PR #186271)

[Mehdi Amini]

joker-eph wrote:

> > It is not OK for any transformation to generate invalid IR from valid IR: that's always a bug.
> 
> I know what you mean, but this particular case raises a wider question for me. The following Op is already invalid

I rather keep terminology strict: the op is not "invalid", it may have undefined behavior or be documented as having runtime checks with abort behavior, or anything you can imagine. But "invalid IR" is strictly "what we reject with the verifier".

> However, I feel that we are missing something at the framework level that would prevent/capture this sort of issues (e.g. **runtime Op verification**).

Can you clarify what you mean by "this sort of issues"?

Note also that we do have support in MLIR for turning undefined behavior into "runtime checks" (think: UBSAN): https://mlir.llvm.org/docs/Passes/#-generate-runtime-verification ; any op can hook there with an interface.

> Hm, the bulider seems to run fine. It's `PackOp::canonicalize() -> paddingIsNotNeeded() -> requirePaddingValue()` that leads to UB, right? 

Oh right, I misread the backtrace and thought the builder was calling `requirePaddingValue()`...

> In fact, how about an assert in `requirePaddingValue`? That method effectively assumes that tile sizes are non-zero.

> * Remove references to `expand_shape` - the relation is not clear to me.
> * [optional] Comments + assert re assumption on tile sizes in `paddingIsNotNeeded()`.

Sure, will do.

> Btw, IMO we are missing a wider discussion on 0-dim in Linalg. That's outside the scope here and I am only mentioning so that we refrain from special-casing for dim-0 if there are more issues like this.

Sure: that's a call Linalg needs to do on whether 0-dim should be well defined behavior: the alternative to my patch here is to relax the verifier further to accept 0-dim and update the client code to accept it.


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