-
Notifications
You must be signed in to change notification settings - Fork 100
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
[CIR][OpenMP] Taskwait, Taskyield and Barrier implementation #555
Conversation
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 the update Walt, this is great. I like the size of the PR and the approach. Few things you need to improve but direction is solid.
In this case it is fine because the body where it's used is not implemented. But otherwise you should assert on the content of those params if it's something that will have an effect on the paths. Therefore i haven't added this line that repeats in multiple runtime calls:
Please add a |
CIRGenFunction::buildOMPTaskwaitDirective(const OMPTaskwaitDirective &S) { | ||
mlir::LogicalResult res = mlir::success(); | ||
// Getting the source location information of AST node S scope | ||
auto scopeLoc = getLoc(S.getSourceRange()); |
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.
Since the locations are only used for param passing, please delete the comment and just add the function call directly while passing the param. Same for the other 2 methods
void CIRGenOpenMPRuntime::emitBarrierCall(CIRGenBuilderTy &builder, | ||
CIRGenFunction &CGF, | ||
mlir::Location Loc) { | ||
if (CGF.CGM.getLangOpts().OpenMPIRBuilder) { |
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.
Still work to be done. Please follow the skeleton, you should either add unrecheables or assert(!Unimplemented::...)
, here's another example for you:
void CGOpenMPRuntime::emitBarrierCall(...) {
// Check if we should use the OMPBuilder
assert(!Unimplemented::openMPRegionInfo());
if (CGF.CGM.getLangOpts().OpenMPIRBuilder) {
builder.create<mlir::omp::BarrierOp>(Loc);
return;
}
if (!CGF.HaveInsertPoint())
return;
llvm_unreacheable("NYI");
}
Please review the other methods and make sure you abide to this approach.
Also please notice this PR is failing to run tests, make sure they pass before pinging for another review round. |
It seems like my local script for executing the tests and formatting the code was outdated, now I've updated the tests with the flag -fopenmp-enable-irbuilder and followed the same exact control flow that OG clang runtime calls follow. Thank you @bcardosolopes ! |
ef561c8
to
b7d1230
Compare
c4db6d0
to
e197d4e
Compare
I've noticed the tests were failing due to changes in the behavior of clang flags (Moreover, if you try to execute clangir on godbolt, it returns an error of unknown flag -fclangir-enable) ,i've changed them through a git commit --amend, I've double-checked everything, thank you! |
We changed |
As you can see it works now just fine, #572 worked because its a IR test and not a code gen test, therefore he didn't need to change the -fclangir-enable to -fclangir I'm guessing. But I'm now wondering, why does this branch has any conflicts at all? That test file of parallel was just renamed, and no one has changed that file. |
system is not that smart, just runs everything |
dispatched another round of tests for this PR, let's see |
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 applying the fixes, LGTM
This PR is the final fix for issue llvm#499.
This PR is the final fix for issue llvm#499.
This PR is the final fix for issue llvm#499.
This PR is the final fix for issue #499.
This PR is the final fix for issue #499. I had to delete both the branch because some mistaken actions were irreversible during the rebase process. Eager to hear some feedback.