-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
[mlir][LLVM] erase call mappings in forgetMapping() #84955
Conversation
@llvm/pr-subscribers-flang-openmp @llvm/pr-subscribers-mlir Author: Tom Eccles (tblah) ChangesIt looks like the mappings for call instructions were forgotten here. This fixes a bug in OpenMP when in-lining a region containing call operations multiple times. OpenMP array reductions 4/6 Full diff: https://github.com/llvm/llvm-project/pull/84955.diff 1 Files Affected:
diff --git a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
index c00628a420a000..995544238e4a3c 100644
--- a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
@@ -716,6 +716,8 @@ void ModuleTranslation::forgetMapping(Region ®ion) {
branchMapping.erase(&op);
if (isa<LLVM::GlobalOp>(op))
globalsMapping.erase(&op);
+ if (isa<LLVM::CallOp>(op))
+ callMapping.erase(&op);
llvm::append_range(
toProcess,
llvm::map_range(op.getRegions(), [](Region &r) { return &r; }));
|
@llvm/pr-subscribers-mlir-llvm Author: Tom Eccles (tblah) ChangesIt looks like the mappings for call instructions were forgotten here. This fixes a bug in OpenMP when in-lining a region containing call operations multiple times. OpenMP array reductions 4/6 Full diff: https://github.com/llvm/llvm-project/pull/84955.diff 1 Files Affected:
diff --git a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
index c00628a420a000..995544238e4a3c 100644
--- a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
@@ -716,6 +716,8 @@ void ModuleTranslation::forgetMapping(Region ®ion) {
branchMapping.erase(&op);
if (isa<LLVM::GlobalOp>(op))
globalsMapping.erase(&op);
+ if (isa<LLVM::CallOp>(op))
+ callMapping.erase(&op);
llvm::append_range(
toProcess,
llvm::map_range(op.getRegions(), [](Region &r) { return &r; }));
|
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.
LGTM. Please wait a day incase others have comments.
We need a test here |
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.
Some minor comments about the test.
atomic { | ||
^bb2(%arg2: !llvm.ptr, %arg3: !llvm.ptr): | ||
%2 = llvm.load %arg3 : !llvm.ptr -> f32 | ||
llvm.atomicrmw fadd %arg2, %2 monotonic : !llvm.ptr, f32 | ||
omp.yield | ||
} |
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.
Can the atomic region be removed?
%c1 = llvm.mlir.constant(1 : i32) : i32 | ||
%0 = llvm.alloca %c1 x i32 : (i32) -> !llvm.ptr | ||
omp.parallel { | ||
omp.wsloop reduction(@add_f32 %0 -> %prv : !llvm.ptr) |
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.
Can the reduction be moved to omp.parallel
? And the omp.wsloop
removed?
f2a649a
to
02550e1
Compare
Sorry about the force push. Github wasn't showing the new commit (02550e1). It is just a rebase onto the target branch. |
83ad738
to
4eef5d2
Compare
OpenMP reduction declare operations can contain FIR code which needs to be lowered to LLVM. With array reductions, these regions can contain more complicated operations which need PreCGRewriting. A similar extra case was already needed for fir::GlobalOp. OpenMP array reductions 3/6 Previous PR: #84953 Next PR: #84955
It looks like the mappings for call instructions were forgotten here. This fixes a bug in OpenMP when inlining a region containing call operations multiple times.
02550e1
to
a726997
Compare
…84954) OpenMP reduction declare operations can contain FIR code which needs to be lowered to LLVM. With array reductions, these regions can contain more complicated operations which need PreCGRewriting. A similar extra case was already needed for fir::GlobalOp. OpenMP array reductions 3/6 Previous PR: llvm#84953 Next PR: llvm#84955
It looks like the mappings for call instructions were forgotten here. This fixes a bug in OpenMP when in-lining a region containing call operations multiple times. OpenMP array reductions 4/6 Previous PR: llvm#84954 Next PR: llvm#84957
…code (llvm#84957) Moving extractSequenceType to FIRType.h so that this can also be used from OpenMP. OpenMP array reductions 5/6 Previous PR: llvm#84955 Next PR: llvm#84958
It looks like the mappings for call instructions were forgotten here. This fixes a bug in OpenMP when in-lining a region containing call operations multiple times.
OpenMP array reductions 4/6
Previous PR: #84954
Next PR: #84957