-
Notifications
You must be signed in to change notification settings - Fork 56
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][OpenMP] Support lowering of host_eval to LLVM IR #179
base: users/skatrak/target-passthrough-02-mlir
Are you sure you want to change the base?
[MLIR][OpenMP] Support lowering of host_eval to LLVM IR #179
Conversation
44b6230
to
719e50c
Compare
e2ee789
to
a9a83c9
Compare
719e50c
to
bc6485b
Compare
a9a83c9
to
332c9c5
Compare
bc6485b
to
ad391a1
Compare
332c9c5
to
0443d95
Compare
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.
Minor comments. I went over the entire PR of course, but I'd prefer if someone more familiar with the SPMD stuff went over it so we are sure it is good.
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
Outdated
Show resolved
Hide resolved
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 @bhandarkar-pranav for taking a look. I should have addressed all your concerns with the last update. Regarding your comment about finding someone else familiar with the SPMD stuff, I happen to also be the guy who made the previous implementation 😅, and unsurprisingly changes look good to myself! 😄
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
Outdated
Show resolved
Hide resolved
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.
@skatrak - Thank you for the changes. LGTM.
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.
OpenMPIRBuilder changes LGTM
No test added?
// Consider the following difficulties (assuming 8-bit signed integers): | ||
// * Adding \p Step to the loop counter which passes \p Stop may overflow: | ||
// DO I = 1, 100, 50 | ||
/// * A \p Step of INT_MIN cannot not be normalized to a positive direction: | ||
// DO I = 100, 0, -128 | ||
updateToLocation(Loc); |
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.
The code below was meant to check basic errors before modifying anything.
It doesn't matter that much where it is, but it should not give the impression that the comment above would apply to updateToLocation
.
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.
Thanks for pointing this out. I moved the call to updateToLocation
below the asserts, as it was before.
ad391a1
to
595f1ae
Compare
This patch updates the MLIR to LLVM IR lowering of `omp.target` to support passing `num_teams`, `num_threads`, `thread_limit` and SPMD loop bounds through the `host_eval` argument of `omp.target`. This replaces the previous implementation where this information was directly attached to the `omp.target` operation rather than captured to be used by the corresponding nested operation.
36f00b9
to
0e07eda
Compare
Thank you @Meinersbur for taking a look. I added a test that is basically copied from an existing one, since that one used to check all corner cases related to the trip count calculation. I don't know if it makes sense to keep both as they are, but let me know what you think. |
What I had in mind was a test where
|
0e07eda
to
a3727a0
Compare
Ah, that makes sense. Sorry for the misunderstanding! I just added an MLIR to LLVM IR translation test for these host evaluated clauses. I thought you were referring to the OMPIRBuilder refactoring because that's what you said you had reviewed 😅.
What I just did was to remove the |
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, thanks
Value *TripCount = OMPBuilder.calculateCanonicalLoopTripCount( | ||
Loc, StartVal, StopVal, StepVal, IsSigned, InclusiveStop); |
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.
That's actually pretty nice
This patch updates the MLIR to LLVM IR lowering of
omp.target
to support passingnum_teams
,num_threads
,thread_limit
and SPMD loop bounds through thehost_eval
argument ofomp.target
.This replaces the previous implementation where this information was directly attached to the
omp.target
operation rather than captured to be used by the corresponding nested operation.The implementation of
TargetOp::getInnermostCapturedOmpOp
is also improved to address bugs in the detection of target SPMD in MLIR, uncovered by changes to the translation to LLVM IR.