[Mlir-commits] [mlir] [mlir][vector] Don't fold in_bounds for negative constant indices (PR #219681)

[Alessandro Potenza]

https://github.com/alepot55 commented:

Read, not built, so nothing below is an execution result.

The two claims in the description hold up:

- The sentence you are relying on is in `VectorOps.td` twice, at the `transfer_read` definition and again at `transfer_write`, so the promise about the starting point is the same on both paths and both of your tests are warranted.
- The subtraction is safe. `isInBounds` returns early on `op.getShapedType().isDynamicDim(indicesIdx)` before reaching it, so `sourceSize` is always a real static extent and never `ShapedType::kDynamic`. Worth saying out loud, because `maxStart = sourceSize - vectorSize` would be signed overflow on `kDynamic` and the guard that prevents it is twelve lines up.

One thing that is not visible from this PR alone. #215340 adds `isProvablyInBounds` in `VectorInferInBounds.cpp`, and it is the same rule again: same `isDynamicDim` guard, same scalable guard, same `maxStart = sourceSize - vectorSize`, same non-negative lower bound. Once both land, the rule that `in_bounds` covers the starting point is implemented in two places, and the canonicalizer's copy is the special case of the pass's copy where the lower and upper bound coincide on a constant.

That is how the bug you are fixing here survived in the first place: nobody was looking at both at once. I do not think the two can simply be merged, since `ValueBoundsConstraintSet` is too expensive to run from a canonicalization. But the four static guards ahead of the bounds query are byte-identical between them and could be one helper, and failing that the two need comments pointing at each other, so the next person fixing one finds the other.

Minor, and only about consistency between your two PRs: `isProvablyInBounds` bails explicitly on `maxStart < 0`, whereas here that case falls out of `*cstOp >= 0 && *cstOp <= maxStart`. Both are correct. The reader has to notice it twice.

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