[MLIR][OpenMP] Handle privatization for global values in MLIR->LLVM translation #104407
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
llvmbot
added
mlir:llvm
mlir
flang
|
@llvm/pr-subscribers-flang-openmp @llvm/pr-subscribers-mlir Author: Kareem Ergawy (ergawy) ChangesFix for #102939. The issues occurs because the CodeExtractor component only collect inputs This commit attempts to fix the issue by adding a flag to the Full diff: https://github.com/llvm/llvm-project/pull/104407.diff 6 Files Affected:
|
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Kareem Ergawy (ergawy) ChangesFix for #102939. The issues occurs because the CodeExtractor component only collect inputs This commit attempts to fix the issue by adding a flag to the Full diff: https://github.com/llvm/llvm-project/pull/104407.diff 6 Files Affected:
|
ergawy
force-pushed
the
private_bug_102939
branch
2 times, most recently
from
5d93023 to
0dd676f
Compare
ergawy
commented
Aug 18, 2024
Comment on lines
+1551
to
+1560
|
|
||
| Extractor.findInputsOutputs(Inputs, Outputs, SinkingCands, | ||
| /*CollectGlobalInputs=*/true); | ||
|
|
||
| Inputs.remove_if([&](Value *I) { | ||
| if (auto *GV = dyn_cast_if_present<GlobalVariable>(I)) | ||
| return GV->getValueType() == OpenMPIRBuilder::Ident; | ||
|
|
||
| return false; | ||
| }); |
There was a problem hiding this comment.
Tbh, I do not know if this is cleanest or best approach to solve this issue. To get a better understanding of the problem, please take a look at the new test added in openmp-firstprivate.mlir below as well as the 2 linked issues: #102939 and #102949.
Let me know if you disagree with the solution and/or have a better suggestion.
ergawy
requested review from
tblah,
kiranchandramohan,
jdoerfert and
bhandarkar-pranav
| Extractor.findInputsOutputs(Inputs, Outputs, SinkingCands, | ||
| /*CollectGlobalInputs=*/true); | ||
|
|
||
| Inputs.remove_if([&](Value *I) { |
There was a problem hiding this comment.
Would it not be better to NOT add the global value in the first place, if it's an OpenMPIRBuilder::Ident? You already do isa in the code below, so doing a GV = dyn_cast_if_present instead and follow that with a getValueType != OpenMPIRBuilder::Ident?
There was a problem hiding this comment.
The problem with that is:
OpenMPIRBuilder::Identare created and maintained by the functions of theOpenMPIRBuilder. So I think it would be preferable to keep the knowledge of these structs within the builder.- The
CodeExtractoris not OpenMP-related and I think it would better not to leak any non-related OpenMP-details to it.
Of course it is better not to add something and then remove it. But I think it logically isolates things better this way.
There was a problem hiding this comment.
@Leporacanthicus is it fine with you merging this PR? Let me know if you do not agree with my reply above.
ergawy
force-pushed
the
private_bug_102939
branch
2 times, most recently
from
2ce7998 to
16f2d26
Compare
…ranslation Fix for llvm#102939. The issues occurs because the CodeExtractor component only collect inputs (to the parallel regions) that are defined in the same function in which the parallel regions is present. Howerver, this is problematic because if we are privatizing a global value (e.g. a `target` variable which is emitted as a global), then we miss finding that input and we do not privatize the variable. This commit attempts to fix the issue by adding a flag to the CodeExtractor so that we can collect global inputs.
ergawy
force-pushed
the
private_bug_102939
branch
from
16f2d26 to
66c94ea
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/179/builds/4972 Here is the relevant piece of the build log for the reference |
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.
Potential fix for #102939 and #102949.
The issues occurs because the CodeExtractor component only collect inputs (to the parallel regions) that are defined in the same function in which the parallel regions is present. Howerver, this is problematic because if we are privatizing a global value (e.g. a
targetvariable which is emitted as a global), then we miss finding that input and we do not privatize the variable.This commit attempts to fix the issue by adding a flag to the CodeExtractor so that we can collect global inputs.