This patch is child of D89671, contains the clang
implementation to use the OpenMP IRBuilder's section
construct.
Co-author: @anchu-rajendran
Paths
| Differential D91054
[Clang][OpenMP] Frontend work for sections - D89671 ClosedPublic Authored by AMDChirag on Nov 9 2020, 1:03 AM.
Details Summary This patch is child of D89671, contains the clang Co-author: @anchu-rajendran
Diff Detail
Event TimelineAMDChirag retitled this revision from [LLVM][OpenMP] Frontend work for sections - D89671 to [Clang][OpenMP] Frontend work for sections - D89671.Nov 9 2020, 1:15 AM Comment Actions Thanks for doing this. Please add proper lit test. you can find that under clang/test/OpenMP/sections_codegen.cpp. add a case for using the OMPBuilder. In case you need it; you can check that your changes works by doing ninja/make -check-clang-openmp Edit:
AMDChirag marked an inline comment as done. Comment ActionsUpdated code according to the changes in LLVM side of things. Test cases are still required, however. Comment Actions Added OMP delegating code for createSection (EmitOMPSectionDirective). Comment Actions Added FIXME comment for cancellation construct not working with sections construct Comment Actions
Let's resolve the issue described in the TODO before we go ahead with this. There is little point reviewing something that can't be tested.
Comment Actions Fixed cancel construct and applied updates to its test cases The test cases are updated so that the IR generated with and without IRBuilder are considered as they are different. The cancel codegenerator with IRBuilder causes the IR to be created in the form: ... %29 = call i32 @__kmpc_cancel(%struct.ident_t* @1, i32 %omp_global_thread_num9, i32 3) %30 = icmp eq i32 %29, 0 ... Whereas, without IRBuilder, the IR created is in the form: ... %20 = call i32 @__kmpc_cancel(%struct.ident_t* @1, i32 %0, i32 3) %21 = icmp ne i32 %20, 0 ... In essence, the output is logically same, but semantically different, hence the requirement of updating test cases. Comment Actions You can update the tests as long as long as the output is correct. for example the difference is only in names, ordering of basicblocks and instructions that doesn't affect correctness, etc. Comment Actions
Yes, the correctness is not affected with the changes to the test case. Comment Actions @Meinersbur For D89671, Johannes had some comments which have been taken care of. I am just waiting for him to get back on it. I'll ping him there. Comment Actions What is missing or not working in the IRBuilder impl that prevents us from turning it on by default? Comment Actions
Per the TODO in this patch (CGStmtOpenMP.cpp - 3775), PrivatizationCallback is not working right now. Comment Actions
OK, we really need to provide the PrivCB impl so we can start removing clang code. Comment Actions
I have none. All good for me Comment Actions
It might be OK to start with the functionality that replaces directives with single runtime calls, but I don't think implementations of code-associated directives are ready enough. Comment Actions
Then LGTM.
Not sure what "not ready enough" means. If the functionality is equivalent we can switch over and remove duplication, if not, we can't. This revision is now accepted and ready to land.Apr 27 2021, 10:05 PM Comment Actions Rebase to retrigger build The build failed previously - seemingly with a test case that has nothing to do with this patch. Closed by commit rGc20410618827: [Clang][OpenMP] Frontend work for sections - D89671 (authored by AMDChirag). · Explain WhyApr 29 2021, 7:22 AM This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 341515 clang/lib/CodeGen/CGOpenMPRuntime.cpp
clang/lib/CodeGen/CGStmtOpenMP.cpp
clang/test/OpenMP/cancel_codegen.cpp
|
Why do we unpack the children here instead of making a single call back for the CapturedStmt?