search

llvm / clangir

A new (MLIR based) high-level IR for clang.
https://clangir.org
Other
327 stars 86 forks source link

[CIR][OpenMP] Taskwait, Taskyield and Barrier implementation #555

Closed eZWALT closed 3 months ago

eZWALT commented 3 months ago

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.

eZWALT commented 3 months ago

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.

bcardosolopes commented 3 months ago

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

eZWALT commented 3 months ago

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 commented 3 months ago

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

eZWALT commented 3 months ago

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 commented 3 months ago

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? https://github.com/llvm/clangir/pull/572 was updated 10h ago and had no such problem. I just dispatched another test run, let's see what that gives.

eZWALT commented 3 months ago

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 commented 3 months ago

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

system is not that smart, just runs everything

bcardosolopes commented 3 months ago

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

eZWALT commented 3 months ago

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 🙏