This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][OpenMPIRBuilder] Migrate kernel launch code and host fallback code generation from Clang to the OpenMPIRBuilder
ClosedPublic

Authored by jsjodin on May 20 2023, 12:13 PM.

Details

Summary

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.

Diff Detail

Event Timeline

jsjodin created this revision.May 20 2023, 12:13 PM
jsjodin requested review of this revision.May 20 2023, 12:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2023, 12:13 PM

@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
9668

Not sure what this means. Will try to remove it.

jsjodin retitled this revision from [OpenMP][OpenMPIRBuilder] Migrate kernel launch code and host fallback code generation from Clang to the OpenMPIRBuilder to [WIP][OpenMP][OpenMPIRBuilder] Migrate kernel launch code and host fallback code generation from Clang to the OpenMPIRBuilder.May 20 2023, 12:17 PM
TIFitis added inline comments.
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
912–947

D146557 already migrates these along with emitIfClause.

jsjodin retitled this revision from [WIP][OpenMP][OpenMPIRBuilder] Migrate kernel launch code and host fallback code generation from Clang to the OpenMPIRBuilder to [OpenMP][OpenMPIRBuilder] Migrate kernel launch code and host fallback code generation from Clang to the OpenMPIRBuilder.Jun 5 2023, 7:52 AM
jsjodin added inline comments.Jun 5 2023, 7:55 AM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
912–947

Okay, I will rebase this once your patch is merged.

jsjodin updated this revision to Diff 532693.Jun 19 2023, 9:22 AM

Use emitBlock and emitBranch functions. Rebase.

Ping for review.

Another ping for a review.

Looks generally fine. Some minor nits. Also, this does not impact any tests?

clang/lib/CodeGen/CGOpenMPRuntime.cpp
9583

Unrelated but I feel we should at least emit a trap. This feels user hostile and there is nothing we can really optimize here.

9747

I don't get this code. If we are reverse offloading we are not (necessarily) on the host, right?
We should not do "something" for things we have not implemented, IMHO. Assert out and ensure SEMA emits an error.

9755

Style, above and below.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
442

At least move it out.

1380

Docs.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
336

We should make this a constant variable in some header.

jsjodin updated this revision to Diff 535436.Jun 28 2023, 9:06 AM

Addressed review feedback.

arsenm added a subscriber: arsenm.Jun 28 2023, 9:06 AM
arsenm added inline comments.
clang/lib/CodeGen/CGOpenMPRuntime.cpp
9583

There's a codegen option to emit unreachable as trap

jsjodin marked an inline comment as not done.Jun 28 2023, 9:11 AM

The tests are unaffected since this is just refactoring and migration.

clang/lib/CodeGen/CGOpenMPRuntime.cpp
9747

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?

jsjodin updated this revision to Diff 535792.Jun 29 2023, 7:36 AM

Clean up some comments. Move kernel args vector function outside of struct.

jdoerfert accepted this revision.Jun 29 2023, 8:41 AM

LG, one suggestion below, I doubt it makes an actual difference though, feel free to ignore.

clang/lib/CodeGen/CGOpenMPRuntime.cpp
9747

Agreed, but please leave a comment. This does not strike me as right.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
1598

I meant this.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
336

That works.

This revision is now accepted and ready to land.Jun 29 2023, 8:41 AM
jsjodin updated this revision to Diff 535931.Jun 29 2023, 11:23 AM

Address feedback.

This revision was landed with ongoing or failed builds.Jun 30 2023, 7:51 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2023, 7:51 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript