[flang] Erase fir.local ops before lowering fir to llvm
#143687
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`fir.local` ops are not supposed to have any uses at this point (i.e. during lowering to LLVM). In case of serialization, the `fir.do_concurrent` users are expected to have been lowered to `fir.do_loop` nests. In case of parallelization, the `fir.do_concurrent` users are expected to have been lowered to the target parallel model (e.g. OpenMP).
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-fir-hlfir @llvm/pr-subscribers-flang-codegen Author: Kareem Ergawy (ergawy) Changes
Full diff: https://github.com/llvm/llvm-project/pull/143687.diff 2 Files Affected:
diff --git a/flang/lib/Optimizer/CodeGen/CodeGen.cpp b/flang/lib/Optimizer/CodeGen/CodeGen.cpp
index 82d960a6fc61e..ecce5c6f9d159 100644
--- a/flang/lib/Optimizer/CodeGen/CodeGen.cpp
+++ b/flang/lib/Optimizer/CodeGen/CodeGen.cpp
@@ -3294,6 +3294,29 @@ struct LoadOpConversion : public fir::FIROpConversion<fir::LoadOp> {
}
};
+struct LocalitySpecifierOpConversion
+ : public fir::FIROpConversion<fir::LocalitySpecifierOp> {
+ using FIROpConversion::FIROpConversion;
+ llvm::LogicalResult
+ matchAndRewrite(fir::LocalitySpecifierOp localizer, OpAdaptor adaptor,
+ mlir::ConversionPatternRewriter &rewriter) const override {
+ auto uses = mlir::SymbolTable::getSymbolUses(
+ localizer, localizer->getParentOfType<mlir::ModuleOp>());
+
+ // `fir.local` ops are not supposed to have any uses at this point (i.e.
+ // during lowering to LLVM). In case of serialization, the
+ // `fir.do_concurrent` users are expected to have been lowered to
+ // `fir.do_loop` nests. In case of parallelization, the `fir.do_concurrent`
+ // users are expected to have been lowered to the target parallel model
+ // (e.g. OpenMP).
+ if (!uses || !uses->empty())
+ return mlir::failure();
+
+ rewriter.eraseOp(localizer);
+ return mlir::success();
+ }
+};
+
/// Lower `fir.no_reassoc` to LLVM IR dialect.
/// TODO: how do we want to enforce this in LLVM-IR? Can we manipulate the fast
/// math flags?
@@ -4249,15 +4272,15 @@ void fir::populateFIRToLLVMConversionPatterns(
FieldIndexOpConversion, FirEndOpConversion, FreeMemOpConversion,
GlobalLenOpConversion, GlobalOpConversion, InsertOnRangeOpConversion,
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);
+ LocalitySpecifierOpConversion, 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 that are populated without a type converter do not trigger
// target materializations for the operands of the root op.
diff --git a/flang/test/Fir/local.fir b/flang/test/Fir/local.fir
new file mode 100644
index 0000000000000..006f5ca944670
--- /dev/null
+++ b/flang/test/Fir/local.fir
@@ -0,0 +1,10 @@
+// RUN: fir-opt --fir-to-llvm-ir %s | FileCheck %s
+
+// Tests that `fir.local` ops are dropped from the module before LLVM lowering.
+
+fir.local {type = local} @local_privatizer : i32
+func.func @foo() {
+ return
+}
+
+// CHECK-NOT: fir.local
|
jeanPerier
reviewed
Jun 11, 2025
| llvm::LogicalResult | ||
| matchAndRewrite(fir::LocalitySpecifierOp localizer, OpAdaptor adaptor, | ||
| mlir::ConversionPatternRewriter &rewriter) const override { | ||
| auto uses = mlir::SymbolTable::getSymbolUses( |
There was a problem hiding this comment.
I am afraid this may be a bit expensive on big programs. I think this will walk the whole IR to check for usages. So, this is likely introducing a number_of_fir_local * number_of_operation_in_module behavior, and number_of_operation_in_module can be quite big in some apps (>1million).
I would unconditionally erase it at that point in the pipeline (and let any bogus later usage complain if it fails to find the symbol), maybe leaving this check as an assert under #ifdef EXPENSIVE_CHECKS.
If at some point we do need the flexibility of this pass to actually leave fir.local in place when still used for some reason, we should use a SymbolUserMap created once and maintained.
There was a problem hiding this comment.
Thanks for the quick review. Done.
tblah
reviewed
Jun 11, 2025
This is a unified location where we can take care of this. Also, since the same |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/157/builds/30566 Here is the relevant piece of the build log for the reference |
tomtor
pushed a commit
to tomtor/llvm-project
that referenced
this pull request
Jun 14, 2025
…43687) `fir.local` ops are not supposed to have any uses at this point (i.e. during lowering to LLVM). In case of serialization, the `fir.do_concurrent` users are expected to have been lowered to `fir.do_loop` nests. In case of parallelization, the `fir.do_concurrent` users are expected to have been lowered to the target parallel model (e.g. OpenMP). This hopefully resolved a build issue introduced by llvm#142567 (see for example: https://lab.llvm.org/buildbot/#/builders/199/builds/4009).
akuhlens
pushed a commit
to akuhlens/llvm-project
that referenced
this pull request
Jun 24, 2025
…43687) `fir.local` ops are not supposed to have any uses at this point (i.e. during lowering to LLVM). In case of serialization, the `fir.do_concurrent` users are expected to have been lowered to `fir.do_loop` nests. In case of parallelization, the `fir.do_concurrent` users are expected to have been lowered to the target parallel model (e.g. OpenMP). This hopefully resolved a build issue introduced by llvm#142567 (see for example: https://lab.llvm.org/buildbot/#/builders/199/builds/4009).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fir.localops are not supposed to have any uses at this point (i.e. during lowering to LLVM). In case of serialization, thefir.do_concurrentusers are expected to have been lowered tofir.do_loopnests. In case of parallelization, thefir.do_concurrentusers are expected to have been lowered to the target parallel model (e.g. OpenMP).This hopefully resolved a build issue introduced by #142567 (see for example: https://lab.llvm.org/buildbot/#/builders/199/builds/4009).