This patch refactors the code generation that emits the offloading kernel launch and moves the core portion to the OpenMPIRBuilder so that it can be used from flang in the future.
Details
Diff Detail
Event Timeline
@jdoerfert Let me know if you think this approach is fine. I pulled out code into static functions to make it easier to reuse the code. I will try to migrate more code related to generateDefaultMapInfo.
clang/lib/CodeGen/CGOpenMPRuntime.cpp | ||
---|---|---|
9673 | Not sure what this means. Will try to remove it. |
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | ||
---|---|---|
901–936 | Okay, I will rebase this once your patch is merged. |
Looks generally fine. Some minor nits. Also, this does not impact any tests?
clang/lib/CodeGen/CGOpenMPRuntime.cpp | ||
---|---|---|
9588 | Unrelated but I feel we should at least emit a trap. This feels user hostile and there is nothing we can really optimize here. | |
9752 | I don't get this code. If we are reverse offloading we are not (necessarily) on the host, right? | |
9760 | Style, above and below. | |
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h | ||
442 | At least move it out. | |
1409 | Docs. | |
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | ||
336 | We should make this a constant variable in some header. |
clang/lib/CodeGen/CGOpenMPRuntime.cpp | ||
---|---|---|
9588 | There's a codegen option to emit unreachable as trap |
The tests are unaffected since this is just refactoring and migration.
clang/lib/CodeGen/CGOpenMPRuntime.cpp | ||
---|---|---|
9752 | The way I understand it is that whatever code that was generated for the device is ignored (since it is wrong) and we only emit the host fallback. I agree this is not the right thing to e.g. it ignores the OMP_TARGET_OFFLOAD environment variable. When refactoring/migrating I avoid changing how the code behaves in case there are tests that rely on this behavior. | |
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h | ||
442 | Not sure exactly what you meant, but I moved this struct inside the OMPIRBuilder since it makes sense for it to use the runtime args struct. | |
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | ||
336 | I put "#define OMP_KERNEL_ARG_VERSION 2" in OMPConstants.h, is that what you had in mind? |
LG, one suggestion below, I doubt it makes an actual difference though, feel free to ignore.
clang/lib/CodeGen/CGOpenMPRuntime.cpp | ||
---|---|---|
9752 | Agreed, but please leave a comment. This does not strike me as right. | |
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h | ||
1636 | I meant this. | |
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | ||
336 | That works. |
Unrelated but I feel we should at least emit a trap. This feels user hostile and there is nothing we can really optimize here.