[flang] Fix flang build after #83132 #83253
Conversation
This fix is a temporary workaround. `LowerHLFIRIntrinsics.cpp` should be using the greedy pattern rewriter or a manual IR traversal. All patterns in this file are rewrite patterns. The test failure was caused by `replaceAllUsesWith`, which is not supported by the dialect conversion; additional asserts were added recently to prevent incorrect API usage. These trigger now. Alternatively, turning the patterns into conversion patterns and specifying a type converter may work. Failing test case: `Fortran/gfortran/regression/gfortran-regression-compile-regression__inline_matmul_14_f90.test`
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Matthias Springer (matthias-springer) ChangesThis fix is a temporary workaround. Alternatively, turning the patterns into conversion patterns and specifying a type converter may work. Failing test case: Full diff: https://github.com/llvm/llvm-project/pull/83253.diff 1 Files Affected:
diff --git a/flang/lib/Optimizer/HLFIR/Transforms/LowerHLFIRIntrinsics.cpp b/flang/lib/Optimizer/HLFIR/Transforms/LowerHLFIRIntrinsics.cpp
index 314e4264c17e83..377cc44392028f 100644
--- a/flang/lib/Optimizer/HLFIR/Transforms/LowerHLFIRIntrinsics.cpp
+++ b/flang/lib/Optimizer/HLFIR/Transforms/LowerHLFIRIntrinsics.cpp
@@ -176,7 +176,14 @@ class HlfirIntrinsicConversion : public mlir::OpRewritePattern<OP> {
rewriter.eraseOp(use);
}
}
- rewriter.replaceAllUsesWith(op->getResults(), {base});
+ // 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.
+ op->getResult(0).replaceAllUsesWith(base);
rewriter.replaceOp(op, base);
}
};
|
|
Btw, how do I run the above mentioned test case? I think this commit should fix it, but I haven't actually verified it locally. There is no file with that name ( |
|
The gfortran regression tests are in the LLVM testsuite repo: https://github.com/llvm/llvm-test-suite/tree/main/Fortran/gfortran I ran the test suite successfully before approving. You could test manually by building and running https://github.com/llvm/llvm-test-suite/blob/main/Fortran/gfortran/regression/inline_matmul_14.f90 with flang. Alternatively you could run the whole test suite. The documentation should be up to date. Please get in touch if any of it is unclear or incorrect. |
In llvm#83253 @matthias-springer pointed out that LowerHLFIRIntrinsics.cpp should not be using rewrite patterns with the dialect conversion driver. The intention of this pass is to lower HLFIR intrinsic operations into FIR and so I think this best fits dialect conversion and so I have changed all of these into conversion patterns. Taking this approach also avoids test suite churn because GreedyPatternRewriter also performs canonicalization. One remaining misuse of the MLIR API is that we replace values of one type with a different (although safe) type e.g. !hlfir.expr<2xi32> -> !hlfir.expr<?xi32>. There isn't a convenient way to perform this conversion in IR at the moment because fir.convert does not accept !hlfir.expr.
In llvm#83253 @matthias-springer pointed out that LowerHLFIRIntrinsics.cpp should not be using rewrite patterns with the dialect conversion driver. The intention of this pass is to lower HLFIR intrinsic operations into FIR so it conceptually fits dialect conversion. However, dialect conversion is much stricter about changing types when replacing operations. This pass sometimes looses track of array bounds, resulting in replacements with operations with different but compatible types (expressions of the same rank and element types but with or without compile time known array bounds). This is difficult to accommodate with the dialect conversion driver and so I have changed to use the greedy pattern rewriter. There is a lot of test churn because the greedy pattern rewriter also performs canonicalization.
) In #83253 @matthias-springer pointed out that LowerHLFIRIntrinsics.cpp should not be using rewrite patterns with the dialect conversion driver. The intention of this pass is to lower HLFIR intrinsic operations into FIR so it conceptually fits dialect conversion. However, dialect conversion is much stricter about changing types when replacing operations. This pass sometimes looses track of array bounds, resulting in replacements with operations with different but compatible types (expressions of the same rank and element types but with or without compile time known array bounds). This is difficult to accommodate with the dialect conversion driver and so I have changed to use the greedy pattern rewriter. There is a lot of test churn because the greedy pattern rewriter also performs canonicalization.
This fix is a temporary workaround.
LowerHLFIRIntrinsics.cppshould be using the greedy pattern rewriter or a manual IR traversal. All patterns in this file are rewrite patterns. The test failure was caused byreplaceAllUsesWith, which is not supported by the dialect conversion; additional asserts were added recently to prevent incorrect API usage. These trigger now.Alternatively, turning the patterns into conversion patterns and specifying a type converter may work.
Failing test case:
Fortran/gfortran/regression/gfortran-regression-compile-regression__inline_matmul_14_f90.test