This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Implement TR8 `present` motion modifier in Clang (1/2)
ClosedPublic

Authored by jdenny on Jul 27 2020, 4:10 PM.

Details

Summary

This patch implements Clang front end support for the OpenMP TR8
present motion modifier for omp target update directives. The
next patch in this series implements OpenMP runtime support.

Diff Detail

Event Timeline

jdenny created this revision.Jul 27 2020, 4:10 PM
ABataev added inline comments.Jul 28 2020, 11:31 AM
clang/lib/Basic/OpenMPKinds.cpp
73–74

Better to check for the new clauses here:

if (OpenMPVersion >= 51 && Type == OMPC_MOTION_MODIFIER_present)
....
clang/lib/Parse/ParseOpenMP.cpp
3462

FIXME. This is a bug.

3476–3477

FIXME. A bug.

jdenny added inline comments.Jul 28 2020, 12:49 PM
clang/lib/Parse/ParseOpenMP.cpp
3462

I think you mean:

  • It's a bug in TR8.
  • I should change TODO to FIXME.
  • This patch implements and tests the TR8 bug, and I should leave it that way for now.

Is all that right?

ABataev added inline comments.Jul 28 2020, 12:51 PM
clang/lib/Parse/ParseOpenMP.cpp
3462

Ah, I misread your comment here. I thought it is a bug in a compiler. Then just leave it as is.

jdenny updated this revision to Diff 281339.Jul 28 2020, 1:16 PM
jdenny marked 4 inline comments as done.

Adjusted logic for rejecting present as requested. Rebased.

jdenny added inline comments.Jul 28 2020, 1:17 PM
clang/lib/Basic/OpenMPKinds.cpp
73–74

I think I've done what you're after here. This now looks a look like the related code for map type modifiers above.

This revision is now accepted and ready to land.Jul 28 2020, 1:41 PM
jdenny added inline comments.Jul 28 2020, 5:54 PM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
8055–8057

I haven't managed to locally reproduce the bot failures I saw today when trying to push this patch. Somehow they're dropping the present modifiers during codegen. I think it's due to memory corruption that my machines aren't managing to experience. The culprit seems to be that MapModifiers is local here, but the InfoGen call below stores it in a MapInfo as an ArrayRef, which becames a dangling ref by the time it's used.

One solution is to change MapInfo to store a SmallVector instead. Another would be for MapInfo to store a second ArrayRef for C->getMotionModifiers(), which would be translated to runtime map flags later. I'm planning to try the latter solution tomorrow. I prefer it because it seems more space efficient and because it translates motion modifiers directly to runtime flags instead of translating to map type modifiers first.

jdenny added inline comments.Jul 29 2020, 9:33 AM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
8055–8057

9f2f3b9de6314a009322b6081c792ebf9a469460 relands this patch with the latter solution.