-
Notifications
You must be signed in to change notification settings - Fork 11.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[flang] Remove materialization workaround in type converter #98743
[flang] Remove materialization workaround in type converter #98743
Conversation
@llvm/pr-subscribers-flang-fir-hlfir @llvm/pr-subscribers-flang-codegen Author: Matthias Springer (matthias-springer) ChangesThis change is in preparation of #97903, which adds extra checks for materializations: it is now enforced that they produce an SSA value of the correct type, so the current workaround no longer works. For Also remove the lowering pattern for Full diff: https://github.com/llvm/llvm-project/pull/98743.diff 3 Files Affected:
diff --git a/flang/include/flang/Tools/CLOptions.inc b/flang/include/flang/Tools/CLOptions.inc
index 7f2910c5cfd3c..7df5044949463 100644
--- a/flang/include/flang/Tools/CLOptions.inc
+++ b/flang/include/flang/Tools/CLOptions.inc
@@ -9,6 +9,7 @@
/// This file defines some shared command-line options that can be used when
/// debugging the test tools. This file must be included into the tool.
+#include "mlir/Conversion/ReconcileUnrealizedCasts/ReconcileUnrealizedCasts.h"
#include "mlir/Conversion/SCFToControlFlow/SCFToControlFlow.h"
#include "mlir/Dialect/LLVMIR/LLVMAttrs.h"
#include "mlir/Pass/PassManager.h"
@@ -223,6 +224,10 @@ inline void addFIRToLLVMPass(
options.forceUnifiedTBAATree = useOldAliasTags;
addPassConditionally(pm, disableFirToLlvmIr,
[&]() { return fir::createFIRToLLVMPass(options); });
+ // The dialect conversion framework may leave dead unrealized_conversion_cast
+ // ops behind, so run reconcile-unrealized-casts to clean them up.
+ addPassConditionally(pm, disableFirToLlvmIr,
+ [&]() { return mlir::createReconcileUnrealizedCastsPass(); });
}
inline void addLLVMDialectToLLVMPass(
diff --git a/flang/lib/Optimizer/CodeGen/CodeGen.cpp b/flang/lib/Optimizer/CodeGen/CodeGen.cpp
index 7483acfcd1ca7..bcd8a8c37c2a5 100644
--- a/flang/lib/Optimizer/CodeGen/CodeGen.cpp
+++ b/flang/lib/Optimizer/CodeGen/CodeGen.cpp
@@ -35,7 +35,6 @@
#include "mlir/Conversion/MathToLLVM/MathToLLVM.h"
#include "mlir/Conversion/MathToLibm/MathToLibm.h"
#include "mlir/Conversion/OpenMPToLLVM/ConvertOpenMPToLLVM.h"
-#include "mlir/Conversion/ReconcileUnrealizedCasts/ReconcileUnrealizedCasts.h"
#include "mlir/Conversion/VectorToLLVM/ConvertVectorToLLVM.h"
#include "mlir/Dialect/Arith/IR/Arith.h"
#include "mlir/Dialect/DLTI/DLTI.h"
@@ -1726,10 +1725,9 @@ struct XReboxOpConversion : public EmboxCommonConversion<fir::cg::XReboxOp> {
// fir.box was translated to an llvm.ptr<llvm.struct<>> and the MLIR pass
// manager inserted a builtin.unrealized_conversion_cast that was inserted
// and needs to be removed here.
- if (isInGlobalOp(rewriter))
- if (auto unrealizedCast =
- loweredBox.getDefiningOp<mlir::UnrealizedConversionCastOp>())
- loweredBox = unrealizedCast.getInputs()[0];
+ if (auto unrealizedCast =
+ loweredBox.getDefiningOp<mlir::UnrealizedConversionCastOp>())
+ loweredBox = unrealizedCast.getInputs()[0];
TypePair inputBoxTyPair = getBoxTypePair(rebox.getBox().getType());
@@ -2669,8 +2667,9 @@ struct TypeDescOpConversion : public fir::FIROpConversion<fir::TypeDescOp> {
};
/// Lower `fir.has_value` operation to `llvm.return` operation.
-struct HasValueOpConversion : public fir::FIROpConversion<fir::HasValueOp> {
- using FIROpConversion::FIROpConversion;
+struct HasValueOpConversion
+ : public mlir::OpConversionPattern<fir::HasValueOp> {
+ using OpConversionPattern::OpConversionPattern;
llvm::LogicalResult
matchAndRewrite(fir::HasValueOp op, OpAdaptor adaptor,
@@ -3515,29 +3514,6 @@ struct MustBeDeadConversion : public fir::FIROpConversion<FromOp> {
}
};
-struct UnrealizedConversionCastOpConversion
- : public fir::FIROpConversion<mlir::UnrealizedConversionCastOp> {
- using FIROpConversion::FIROpConversion;
-
- llvm::LogicalResult
- matchAndRewrite(mlir::UnrealizedConversionCastOp op, OpAdaptor adaptor,
- mlir::ConversionPatternRewriter &rewriter) const override {
- assert(op.getOutputs().getTypes().size() == 1 && "expect a single type");
- mlir::Type convertedType = convertType(op.getOutputs().getTypes()[0]);
- if (convertedType == adaptor.getInputs().getTypes()[0]) {
- rewriter.replaceOp(op, adaptor.getInputs());
- return mlir::success();
- }
-
- convertedType = adaptor.getInputs().getTypes()[0];
- if (convertedType == op.getOutputs().getType()[0]) {
- rewriter.replaceOp(op, adaptor.getInputs());
- return mlir::success();
- }
- return mlir::failure();
- }
-};
-
struct ShapeOpConversion : public MustBeDeadConversion<fir::ShapeOp> {
using MustBeDeadConversion::MustBeDeadConversion;
};
@@ -3714,7 +3690,8 @@ class FIRToLLVMLowering
signalPassFailure();
}
- // Run pass to add comdats to functions that have weak linkage on relevant platforms
+ // Run pass to add comdats to functions that have weak linkage on relevant
+ // platforms
if (fir::getTargetTriple(mod).supportsCOMDAT()) {
mlir::OpPassManager comdatPM("builtin.module");
comdatPM.addPass(mlir::LLVM::createLLVMAddComdats());
@@ -3789,16 +3766,17 @@ void fir::populateFIRToLLVMConversionPatterns(
DivcOpConversion, EmboxOpConversion, EmboxCharOpConversion,
EmboxProcOpConversion, ExtractValueOpConversion, FieldIndexOpConversion,
FirEndOpConversion, FreeMemOpConversion, GlobalLenOpConversion,
- GlobalOpConversion, HasValueOpConversion, InsertOnRangeOpConversion,
- InsertValueOpConversion, IsPresentOpConversion, LenParamIndexOpConversion,
- LoadOpConversion, MulcOpConversion, NegcOpConversion,
- NoReassocOpConversion, SelectCaseOpConversion, SelectOpConversion,
- SelectRankOpConversion, SelectTypeOpConversion, ShapeOpConversion,
- ShapeShiftOpConversion, ShiftOpConversion, SliceOpConversion,
- StoreOpConversion, StringLitOpConversion, SubcOpConversion,
- TypeDescOpConversion, TypeInfoOpConversion, UnboxCharOpConversion,
- UnboxProcOpConversion, UndefOpConversion, UnreachableOpConversion,
- UnrealizedConversionCastOpConversion, XArrayCoorOpConversion,
+ GlobalOpConversion, InsertOnRangeOpConversion, InsertValueOpConversion,
+ IsPresentOpConversion, LenParamIndexOpConversion, LoadOpConversion,
+ MulcOpConversion, NegcOpConversion, NoReassocOpConversion,
+ SelectCaseOpConversion, SelectOpConversion, SelectRankOpConversion,
+ SelectTypeOpConversion, ShapeOpConversion, ShapeShiftOpConversion,
+ ShiftOpConversion, SliceOpConversion, StoreOpConversion,
+ StringLitOpConversion, SubcOpConversion, TypeDescOpConversion,
+ TypeInfoOpConversion, UnboxCharOpConversion, UnboxProcOpConversion,
+ UndefOpConversion, UnreachableOpConversion, XArrayCoorOpConversion,
XEmboxOpConversion, XReboxOpConversion, ZeroOpConversion>(converter,
options);
+
+ patterns.insert<HasValueOpConversion>(patterns.getContext());
}
diff --git a/flang/test/Fir/basic-program.fir b/flang/test/Fir/basic-program.fir
index 7bbfd709b0aaf..dda4f32872fef 100644
--- a/flang/test/Fir/basic-program.fir
+++ b/flang/test/Fir/basic-program.fir
@@ -119,4 +119,5 @@ func.func @_QQmain() {
// PASSES-NEXT: (S) 0 num-dce'd - Number of operations eliminated
// PASSES-NEXT: TargetRewrite
// PASSES-NEXT: FIRToLLVMLowering
+// PASSES-NEXT: ReconcileUnrealizedCasts
// PASSES-NEXT: LLVMIRLoweringPass
|
b73cf35
to
b105e0e
Compare
This change is in preparation of #97903, which adds extra checks for materializations: it is now enforced that they produce an SSA value of the correct type, so the current workaround no longer works. For `fir.has_value` the fix is simple: no target materializations on the operands are performed if the lowering patterns is initialized without a type converter. For `cg::XEmboxOp`, the existing workaround that skips `unrealized_conversion_cast` ops can be generalized. (This is still a workaround.) Also remove the lowering pattern for `unrealized_conversion_cast`. This pattern has no effect because `unrealized_conversion_cast` ops that are inserted by the dialect conversion framework are never matched by the pattern driver.
b105e0e
to
d4963de
Compare
Note: Type conversions do not know the context in which they are converting a type. But each pattern can be initialized with a different type converter. That's the basic idea behind this PR. I tried the same for the workaround in // Inside a fir.global, the input box was produced as an llvm.struct<>
// because objects cannot be handled in memory inside a fir.global body that
// must be constant foldable. However, the type translation are not
// contextual, so the fir.box<T> type of the operation that produced the
// fir.box was translated to an llvm.ptr<llvm.struct<>> and the MLIR pass
// manager inserted a builtin.unrealized_conversion_cast that was inserted
// and needs to be removed here.
if (isInGlobalOp(rewriter))
if (auto unrealizedCast =
loweredBox.getDefiningOp<mlir::UnrealizedConversionCastOp>())
loweredBox = unrealizedCast.getInputs()[0]; Maybe the type conversion in that pattern is truly context-specific. |
Yes, the handling of the fir.box type (Fotran descriptor) is context specific. It is lowered to an "unique pointer" to a struct type, except inside globals, where it is directly lowered to struct because memory cannot be manipulated in a static initializer and what we want to produce is the struct value itself. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @matthias-springer, looks good to me.
…tion (#97903) This commit fixes a bug in the dialect conversion. During a 1:N signature conversion, the dialect conversion did not insert a cast back to the original block argument type, producing invalid IR. See `test-block-legalization.mlir`: Without this commit, the operand type of the op changes because an `unrealized_conversion_cast` is missing: ``` "test.consumer_of_complex"(%v) : (!llvm.struct<(f64, f64)>) -> () ``` To implement this fix, it was necessary to change the meaning of argument materializations. An argument materialization now maps from the new block argument types to the original block argument type. (It now behaves almost like a source materialization.) This also addresses a `FIXME` in the code base: ``` // FIXME: The current argument materialization hook expects the original // output type, even though it doesn't use that as the actual output type // of the generated IR. The output type is just used as an indicator of // the type of materialization to do. This behavior is really awkward in // that it diverges from the behavior of the other hooks, and can be // easily misunderstood. We should clean up the argument hooks to better // represent the desired invariants we actually care about. ``` It is no longer necessary to distinguish between the "output type" and the "original output type". Most type converter are already written according to the new API. (Most implementations use the same conversion functions as for source materializations.) One exception is the MemRef-to-LLVM type converter, which materialized an `!llvm.struct` based on the elements of a memref descriptor. It still does that, but casts the `!llvm.struct` back to the original memref type. The dialect conversion inserts a target materialization (to `!llvm.struct`) which cancels out with the other cast. This commit also fixes a bug in `computeNecessaryMaterializations`. The implementation did not account for the possibility that a value was replaced multiple times. E.g., replace `a` by `b`, then `b` by `c`. This commit also adds a transform dialect op to populate SCF-to-CF patterns. This transform op was needed to write a test case. The bug described here appears only during a complex interplay of 1:N signature conversions and op replacements. (I was not able to trigger it with ops and patterns from the `test` dialect without duplicating the `scf.if` pattern.) Note for LLVM integration: Make sure that all `addArgument/Source/TargetMaterialization` functions produce an SSA of the specified type. Depends on #98743.
This change is in preparation of #97903, which adds extra checks for materializations: it is now enforced that they produce an SSA value of the correct type, so the current workaround no longer works.
The original workaround avoided target materializations by directly returning the to-be-converted SSA value from the materialization callback. This can be avoided by initializing the lowering patterns that insert the materializations without a type converter. For
cg::XEmboxOp
, the existing workaround that skipsunrealized_conversion_cast
ops is still in place.Also remove the lowering pattern for
unrealized_conversion_cast
. This pattern has no effect becauseunrealized_conversion_cast
ops that are inserted by the dialect conversion framework are never matched by the pattern driver.