Skip to content
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

Merged
merged 4 commits into from
May 1, 2024

Conversation

eZWALT
Copy link
Contributor

@eZWALT eZWALT commented Apr 20, 2024

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.

Copy link
Member

@bcardosolopes bcardosolopes left a 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.

clang/lib/CIR/CodeGen/CIRGenFunction.h Outdated Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenStmtOpenMP.cpp Outdated Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenOpenMPRuntime.h Outdated Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenOpenMPRuntime.cpp Outdated Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenOpenMPRuntime.h Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenStmtOpenMP.cpp Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenStmtOpenMP.cpp Outdated Show resolved Hide resolved
@eZWALT
Copy link
Contributor Author

eZWALT commented Apr 25, 2024

also I've ommited the extra parameters of the runtime calls.

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:

  if (auto *Region = dyn_cast_or_null<CGOpenMPRegionInfo>(CGF.CapturedStmtInfo))
    Region->emitUntiedSwitch(CGF);

Please add a assert(!Unimplemented...), as I mentioned in one of the comments.

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());
Copy link
Member

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) {
Copy link
Member

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.

@bcardosolopes
Copy link
Member

Also please notice this PR is failing to run tests, make sure they pass before pinging for another review round.

@eZWALT
Copy link
Contributor Author

eZWALT commented Apr 27, 2024

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 !

@bcardosolopes
Copy link
Member

@eZWALT @lanza rebased over the weekend, sorry for the churn. Can you rebase this PR again?

@bcardosolopes bcardosolopes reopened this Apr 30, 2024
@eZWALT
Copy link
Contributor Author

eZWALT commented May 1, 2024

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!

@bcardosolopes
Copy link
Member

bcardosolopes commented May 1, 2024

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 -fclangir-enable to -fclangir, seems like testing is working on other PRs so we probably fixed it already. Are you sure you are on most updated branch? #572 was updated 10h ago and had no such problem. I just dispatched another test run, let's see what that gives.

@eZWALT
Copy link
Contributor Author

eZWALT commented May 1, 2024

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 -fclangir-enable to -fclangir, seems like testing is working on other PRs so we probably fixed it already. Are you sure you are on most updated branch? #572 was updated 10h ago and had no such problem. I just dispatched another test run, let's see what that gives.

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.

@bcardosolopes
Copy link
Member

worked because its a IR test and not a code gen test

system is not that smart, just runs everything

@bcardosolopes
Copy link
Member

dispatched another round of tests for this PR, let's see

@eZWALT
Copy link
Contributor Author

eZWALT commented May 1, 2024

dispatched another round of tests for this PR, let's see

Thank you for the agility, I've figured out looking at #572 that I was behind a couple of relevant commits, now it shouldn't be a problem, thank you 🙏

Copy link
Member

@bcardosolopes bcardosolopes left a 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

@bcardosolopes bcardosolopes merged commit c9b4fdc into llvm:main May 1, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants