[Mlir-commits] [mlir] [MLIR][Conversion] PDLToPDLInterp: Don't shadow maximum depth position when creating constraint position. (PR #162265)

[llvmlistbot at llvm.org]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-mlir

Author: None (jumerckx)

<details>
<summary>Changes</summary>

`getConstraintPredicates` generates an EqualToConstraint if a constraint result is used elsewhere. There is logic that decides at which position this should occur: `comparePosDepth(second, first)`
```cpp
  // Push the constraint to the furthest position.
  Position *pos = *llvm::max_element(allPositions, comparePosDepth);
  ResultRange results = op.getResults();
  PredicateBuilder::Predicate pred = builder.getConstraint(
      op.getName(), allPositions, SmallVector<Type>(results.getTypes()),
      op.getIsNegated());

  // For each result register a position so it can be used later
  for (auto [i, result] : llvm::enumerate(results)) {
    ConstraintQuestion *q = cast<ConstraintQuestion>(pred.first);
    ConstraintPosition *pos = builder.getConstraintPosition(q, i);
    auto [it, inserted] = inputs.try_emplace(result, pos);
    // If this is an input value that has been visited in the tree, add a
    // constraint to ensure that both instances refer to the same value.
    if (!inserted) {
      Position *first = pos;
      Position *second = it->second;
      if (comparePosDepth(second, first))
        std::tie(second, first) = std::make_pair(first, second);

      predList.emplace_back(second, builder.getEqualTo(first));
    }

```
I believe this is not the intended behavior since `pos` at this point is a ConstraintPosition. This means that, when `comparePosDepth` is later called, the depth will always be zero.

This pr makes it such that the original `pos` defined above is not shadowed by the new one. And the depth comparison is not made with the NativeConstraint's result position, but the deepest position of the operands of the NativeConstraint. I believe this is the intended behavior because this way the position of the "deepest" operand to the native constraint will be considered.

---
Full diff: https://github.com/llvm/llvm-project/pull/162265.diff


1 Files Affected:

- (modified) mlir/lib/Conversion/PDLToPDLInterp/PredicateTree.cpp (+4-4) 


``````````diff
diff --git a/mlir/lib/Conversion/PDLToPDLInterp/PredicateTree.cpp b/mlir/lib/Conversion/PDLToPDLInterp/PredicateTree.cpp
index 39d4815dc73b7..e95a178c030d4 100644
--- a/mlir/lib/Conversion/PDLToPDLInterp/PredicateTree.cpp
+++ b/mlir/lib/Conversion/PDLToPDLInterp/PredicateTree.cpp
@@ -279,14 +279,14 @@ static void getConstraintPredicates(pdl::ApplyNativeConstraintOp op,
   // For each result register a position so it can be used later
   for (auto [i, result] : llvm::enumerate(results)) {
     ConstraintQuestion *q = cast<ConstraintQuestion>(pred.first);
-    ConstraintPosition *pos = builder.getConstraintPosition(q, i);
-    auto [it, inserted] = inputs.try_emplace(result, pos);
+    ConstraintPosition *constraintPos = builder.getConstraintPosition(q, i);
+    auto [it, inserted] = inputs.try_emplace(result, constraintPos);
     // If this is an input value that has been visited in the tree, add a
     // constraint to ensure that both instances refer to the same value.
     if (!inserted) {
-      Position *first = pos;
+      Position *first = constraintPos;
       Position *second = it->second;
-      if (comparePosDepth(second, first))
+      if (comparePosDepth(second, pos))
         std::tie(second, first) = std::make_pair(first, second);
 
       predList.emplace_back(second, builder.getEqualTo(first));

``````````

</details>


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