[flang-commits] [flang] [flang][hlfir][NFC] Fix mlir misuse in LowerHLFIRIntrinsics (PR #83293)

[Matthias Springer via flang-commits]

================
@@ -176,13 +176,9 @@ class HlfirIntrinsicConversion : public mlir::OpRewritePattern<OP> {
           rewriter.eraseOp(use);
       }
     }
-    // TODO: This entire pass should be a greedy pattern rewrite or a manual
-    // IR traversal. A dialect conversion cannot be used here because
-    // `replaceAllUsesWith` is not supported. Similarly, `replaceOp` is not
-    // suitable because "op->getResult(0)" and "base" can have different types.
-    // In such a case, the dialect conversion will attempt to convert the type,
-    // but no type converter is specified in this pass. Also note that all
-    // patterns in this pass are actually rewrite patterns.
+    // the types might not match exactly (but are safe)
+    // e.g. !hlfir.expr<?xi32> vs !hlfir.expr<2xi32>
+    // TODO: is this allowed by MLIR?
----------------
matthias-springer wrote:

Yes, I think this is allowed. (As long as you don't produce an invalid operation, everything's fine.)

The problem in the current implementation is that `op->getResult(0).replaceAllUsesWith(base);` bypasses the rewriter API, i.e., making an IR modification without going through the rewriter. The rewriter equivalent would be `RewriterBase::replaceAllUsesWith`. But that function is not supported by the conversion pattern rewriter yet.

But it should also not be necessary:
```
    op->getResult(0).replaceAllUsesWith(base);
    rewriter.replaceOp(op, base);
```
`rewriter.replaceOp` already performs the replacement, so the previous `replaceAllUsesWith` is redundant. I tried removing the `replaceAllUsesWith` but then the dialect conversion no longer succeeds. I think the problem is that whenever the type changes during a `rewriter.replaceOp`, the dialect conversion tries to build a type conversion op with the type converter. In this case here, no type conversion is needed, but that's not something that the dialect conversion considers. The error that I saw was due to the fact that no type converter is specified. (Take a look at the `ConversionOpPattern` constructor, you can pass a type converter.) One thing you could try is passing a type converter that always returns the same SSA value, i.e., does not convert anything. Not sure if it will work.


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