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.
As an example look in the same directory for parallel_codegen.cpp , master_codegen.cpp , critical_codegen.cpp
In case you need it; you can check that your changes works by doing ninja/make -check-clang-openmp
NIT: Also please change the title of the patch to be more descriptive of the contents of the patch (e.g. [clang][openmp] using OpenmpIRBuilder to implement openmp sections ) or anything you choose :)
You are correct - I could not find a scenario where CS would be a nullptr.
Added FIXME comment for cancellation construct not working with sections construct
Also updated the lit test cases to reflect this change.
The lit test case modification will be removed once this issue is resolved.
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.
Why do we unpack the children here instead of making a single call back for the CapturedStmt?
|14 ↗||(On Diff #319095)|
I don't get it. Where is EmitOMPRegionBody called on the <xyz>.cncl block such that it would remove the terminator?
Maybe you could share the IR at the different levels if you need help resolving the problem.
Because there is a bunch of additional IR (creation of switch statement and its cases/blocks, loop counter based on count of children) that is generated by createSections. If the children are not unpacked and CapturedStmt is directly passed to EmitOMPRegionBody in the callback, createSections will not be able to generate the required IR to encase the section constructs with.
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.
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.
Not sure what "not ready enough" means. If the functionality is equivalent we can switch over and remove duplication, if not, we can't.
Take omp master, which has associated code, but for which I'm not aware of a reason why we can't switch over. We should double check and if we can't find a problem we do it.
There is no point in waiting if we don't look at / work on the stuff.