Page MenuHomePhabricator

[OpenMP] Remove simplified device runtime handling
ClosedPublic

Authored by jhuber6 on Sep 13 2022, 12:44 PM.

Details

Summary

The old device runtime had a "simplified" version that prevented many of
the runtime features from being initialized. The old device runtime was
deleted in LLVM 14 and is no longer in use. Selectively deactivating
features is now done using specific flags rather than the old technique.
This patch simply removes the extra logic required for handling the old
simple runtime scheme.

Diff Detail

Event Timeline

jhuber6 created this revision.Sep 13 2022, 12:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2022, 12:44 PM
jhuber6 requested review of this revision.Sep 13 2022, 12:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2022, 12:44 PM

This is a good idea. Thanks Joseph.

Other than the two comments I made, I think this should be accepted.

Jose

clang/include/clang/Driver/Options.td
2564–2565

Why not remove these? Are they used somewhere else?

clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
77–84

What if we combine these two and just leave one that receives two modes. I am really confused by this code. Is there something I am missing here?

jhuber6 added inline comments.Sep 13 2022, 1:00 PM
clang/include/clang/Driver/Options.td
2564–2565

We usually don't remove driver arguments between releases as this could cause existing applications to stop compiling. Leaving them here will cause Clang to continue compiling but emit an unused flag warning.

clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
77–84

Just me being lazy, I'll combine it into a single one.

Remove the option and let's also remove the device flag too while we are at it.

clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
75–76

Remove

79–80
josemonsalve2 added inline comments.Sep 13 2022, 1:05 PM
clang/include/clang/Driver/Options.td
2564–2565

Will that generate a warning saying this flag has no use?

jhuber6 updated this revision to Diff 459853.Sep 13 2022, 1:11 PM

Removing the flag as per Johanness' suggestion. This will break old build
configruations using this flag, but it should be easy to work around in the
worst case.

jdoerfert added inline comments.Sep 13 2022, 1:35 PM
clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
1048

So, follow up for this?

jhuber6 added inline comments.Sep 13 2022, 1:36 PM
clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
1048

Yes, it changes a lot of tests so I wanted to make it a separate patch.

jhuber6 updated this revision to Diff 459865.Sep 13 2022, 1:37 PM

Fix typo in options.

jdoerfert accepted this revision.Sep 13 2022, 1:44 PM

LG, two comments.

clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
1048

Talking about tests, if you pick false here less changes happen, I think.

clang/test/OpenMP/nvptx_force_full_runtime_SPMD_codegen.cpp
9

Delete this test.

This revision is now accepted and ready to land.Sep 13 2022, 1:44 PM
jhuber6 marked an inline comment as done.Sep 13 2022, 1:55 PM
jhuber6 added inline comments.
clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
1048

It's mostly meaningless as I'll remove it later anyway. I figured changing it to true was closer to what this change means as we always use the full runtime.

jhuber6 updated this revision to Diff 459881.Sep 13 2022, 2:48 PM

Updating test

This revision was automatically updated to reflect the committed changes.