[Mlir-commits] [mlir] [MLIR][SCF] Handle commuted indices in parallel loop fusion (PR #219665)

[Alessandro Potenza]

https://github.com/alepot55 commented:

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

**The callback runs for every commutative op, not just `arith.addi`, and a different arity now means "not equivalent".**

In `OperationSupport.cpp` the callback is reached like this:

```cpp
if (!(flags & IgnoreCommutativity) && checkCommutativeEquivalent &&
    lhs->hasTrait<mlir::OpTrait::IsCommutative>()) {
  ...
  if (failed(checkCommutativeEquivalent(lhsRange, rhsRange)))
    return false;
} else {
  // Check pair wise for equivalence.
```

So the guard is the `IsCommutative` trait, not the op name, and a `failure()` from the callback returns `false` outright: the pairwise loop in the `else` is not a fallback, it is the branch taken only when no callback was supplied. This patch returns `failure()` whenever either range is not exactly two operands, so any commutative op of a different arity that used to compare equal pairwise now compares unequal, and a fusion that used to happen stops happening.

I could not name a variadic commutative op that actually reaches `valsAreEquivalent` today, so this may be latent rather than live. It still costs one line to not have it:

```cpp
if (lhs.size() != rhs.size())
  return failure();
if (lhs.size() != 2)
  return success(llvm::all_of(llvm::zip_equal(lhs, rhs), [&](auto p) {
    return valsAreEquivalent(std::get<0>(p), std::get<1>(p), loopsIVsMap);
  }));
```

**The framework already has a general version of this.** The equivalence cache in `OperationSupport.cpp` has its own `checkCommutativeEquivalent`: it rejects on size mismatch, tries the operands in order, and if that fails sorts both ranges by the equivalence-class representative and compares. That is the n-ary form of what this patch writes for n = 2, and the header says the callback exists so that a caller "can query the optional equivalence classes", which is exactly why `ParallelLoopFusion` has to bring its own rather than reuse that one.

Worth a sentence in the PR saying so, and worth asking whether the new lambda should follow the same shape (size equality, then in-order, then the swap) so the two do not drift. Right now the patch tries in-order first and swapped second, which matches, but it drops the size-equality case into a hard failure where the default returns failure only on a genuine mismatch.

**Smaller: the title and description say `arith.addi`, and the code says every commutative op.** That gap is what hides the point above. `Fixes #218614` describes the `%iv + 1` versus `1 + %iv` case, but the change is broader than the fix it claims.

No reviewers are requested on this.

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