[MLIR][Conversion] PDLToPDLInterp: Don't shadow maximum depth position when creating constraint position.

[jumerckx]

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)

  // 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.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[jumerckx]

Ping

[llvmbot]

@llvm/pr-subscribers-mlir

Author: None (jumerckx)

Changes

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)

  // 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 --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));