[CIR][OpenMP] Initial implementation of taskwait, taskyield and barrier #510
Conversation
There was a problem hiding this comment.
This is great, thanks for working on openmp support.
PR looks mostly good!
We try to follow the LLVM codegen skeleton as much as possible, that means that as part of adding these small functionalities, we have to add the skeleton as we go, so it becomes pretty clear where to continue development. See my comments for buildDependences and CIRGenOpenMPRuntime below. You should start mocking those up, shortcuting with llvm_unreacheable and assert(!UnimplementedFeature::someMissingFeature()) for all paths that are not yet available. If you look those up in the CIR codebase you'll find many examples. Let me know if you have questions!
|
|
||
| mlir::LogicalResult | ||
| CIRGenFunction::buildOMPTaskwaitDirective(const OMPTaskwaitDirective &S) { | ||
| mlir::LogicalResult res = mlir::success(); |
There was a problem hiding this comment.
Looking at the traditional LLVM codegen in Clang, I see:
OMPTaskDataTy Data;
// Build list of dependences
buildDependences(S, Data);
Data.HasNowaitClause = S.hasClausesOfKind<OMPNowaitClause>();
CGM.getOpenMPRuntime().emitTaskwaitCall(*this, S.getBeginLoc(), Data);
For the most part, the CIRGen code should look very similar to this, with the core of buildDependences and emitTaskwaitCall doing the actual MLIR based openMP support in CIR. A good start is to add a simple version of buildDependences and implement the minimal CIRGenOpenMPRuntime needed for a emitTaskwaitCall to build the omp dialect operations.
There was a problem hiding this comment.
Okey now I'm understanding the underlying issue of my implementation. OpenMP has a runtime library that has to perform some checks before emitting certain instructions like taskwait. To continue the implementation I wanted to ask you If it would be a better idea to reimplement data structures like OMPTaskDataTy on ClangIR or to just include them. I would personally include those said structures, but I'm not sure at all. So my question is: Do i add the CGOpenMPRuntime.h to the clang/include dir, or do I rewrite the data structures from scratch inside ClangIR?
Also I wanted to point out that almost every PR's is bugged for some reason.
There was a problem hiding this comment.
To continue the implementation I wanted to ask you If it would be a better idea to reimplement data structures like OMPTaskDataTy on ClangIR or to just include them
Reimplement, CIR doesn't have access to lib/CodeGen. The idea is to add the minimal skeleton for OMPTaskDataTy you need for this PR, and for other data structures and pieces you might need for CGOpenMPRuntime.h. The idea is not to blindly copy it, but add the pieces incrementally in each PR, as the need appears.
Do i add the CGOpenMPRuntime.h to the clang/include dir, or do I rewrite the data structures from scratch inside ClangIR?
Rewrite them, we already do that a bunch, so you can look at the existing stuff for inspiration.
Also I wanted to point out that almost every PR's is bugged for some reason
This happens when we rebase the clangir repo against LLVM upstream. You need to rebase your local work against our repo and force push the PR to fix it.
There was a problem hiding this comment.
I've been trying to rebase for a while, but I think I got thinks mixed up, is there any guide or documentation to understand your workflow approach? I've been trying to do the following, but I'm still getting the same message of thousands of commits behind and forward (Less than before). I think i'm missing something.
git rebase upstream/main
git push --force-with-lease
There was a problem hiding this comment.
I finally got to rebase the branch correctly, but now It seems that there is some duplicity on the commits. I'm trying to address this issue using git rebase -i and squashing the commits together but for some reason I can't do that without getting merge conflicts. Appart from that, could you @bcardosolopes review the last minor changes I've uploaded for OpenMPRuntime? I kept them to minimals except for buildDependences, which I thought that was convenient to use the original Clang one.
There was a problem hiding this comment.
Interesting, not sure what's going on - people do it all the time and it usually works, make sure you are rebasing against clangir's repo upstream, not LLVM's main repo upstream.
If you cannot make the rebase happen, feel free to start a new PR (after you apply the requested changes).
Appart from that, could you @bcardosolopes review the last minor changes I've uploaded for OpenMPRuntime?
I rather look into a clean PR instead.
clang/test/CIR/CodeGen/barrier.cpp
Outdated
| @@ -0,0 +1,8 @@ | |||
| // RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -fopenmp -fclangir-enable -emit-cir %s -o %t.cir | |||
| // RUN: FileCheck --input-file=%t.cir %s | |||
There was a problem hiding this comment.
Can you create a subdir for these tests? CIR/CodeGen/OpenMP/barrier.cpp, etc.
There was a problem hiding this comment.
That was my original intention. On a second thought, moving and renaming the openmp.cpp file that @fabianmcg created for parallel, may cause version issues for him, so first I want to ask for permission. @fabianmcg what do you think?
There was a problem hiding this comment.
This should be fine, he can rebase and move it down to the subdir, this is normal github workflow :)
clang/test/CIR/Lowering/barrier.cir
Outdated
| @@ -0,0 +1,14 @@ | |||
| // RUN: cir-translate %s -cir-to-llvmir | FileCheck %s | |||
|
|
|||
There was a problem hiding this comment.
Probably good to add a OpenMP subdir here too.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Gentle ping: any updates on this one? |
I wanted to let you know that due to some issues with my local Git setup, I had to close the last PR and reopen it. I apologize for any inconvenience this may cause. Some of the features mentioned in #499 have been thoroughly reviewed and are ready to go. Thank you for your patience!