This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][AMDGCN] Support OpenMP offloading for AMDGCN architecture - Part 2
ClosedPublic

Authored by saiislam on Jun 1 2020, 4:12 AM.

Details

Summary

New file include to support platform dependent grid constants. It will be
used by clang, libomptarget plugins, and deviceRTLs to access constant
values consistently and with fast access in the deviceRTLs.

Originally authored by Greg Rodgers (@gregrodgers).

Diff Detail

Event Timeline

saiislam created this revision.Jun 1 2020, 4:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2020, 4:12 AM
arsenm added inline comments.Jun 1 2020, 10:11 AM
clang/include/clang/Basic/OpenMPGridValues.h
96 ↗(On Diff #267580)

What about wave32?

clang/include/clang/Basic/TargetInfo.h
1331

long long is the worst C type and should never be used. int64_t?

clang/lib/Basic/Targets/AMDGPU.cpp
290

Why do these need casts?

jdoerfert added inline comments.Jun 1 2020, 10:45 AM
clang/include/clang/Basic/OpenMPGridValues.h
134 ↗(On Diff #267580)

Can we move this into llvm core (subfolder Frontend/OpenMP/)? We probably need this during OpenMPOpt, and for Flang as well.

saiislam updated this revision to Diff 268264.Jun 3 2020, 11:26 AM
  1. Added wave32 as an additional alternate wave size.
  2. Replaced long long with uint64_t and int with unsigned.
  3. Removed unnecessary casts.
  4. Moved OpenMPGridValues.h to Frontend/OpenMP.
  5. Refactored OpenMPGridValues.h as OMPGridValues.h to follow style of neighboring files.
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2020, 11:26 AM
arsenm added inline comments.Jun 3 2020, 12:04 PM
clang/lib/Basic/Targets/AMDGPU.cpp
290

Don't need parentheses

clang/lib/Basic/Targets/NVPTX.cpp
66

Don't need parentheses

jdoerfert added inline comments.Jun 3 2020, 1:58 PM
clang/include/clang/Basic/TargetInfo.h
204

I would recommend adding a descriptive comment, even though that seems not to be the norm here.
Why do we split these into "normal" and "long" grid values again?

= nullptr and an assert in the getters?

llvm/include/llvm/Frontend/OpenMP/OMPGridValues.h
21

I would recommend omp not GPU, or omp before gpu.

saiislam updated this revision to Diff 269314.Jun 8 2020, 12:15 PM
  1. Renamed the GPU namespace as omp.
  2. Added assert in getter functions.
  3. Added comments.
  4. Removed unnecassary parantheses.
saiislam marked 7 inline comments as done.Jun 8 2020, 12:17 PM
saiislam added inline comments.
clang/include/clang/Basic/TargetInfo.h
204

Realized no practical use of separate long values. Folded into regular enum and arrays.

jdoerfert added inline comments.Jun 8 2020, 5:00 PM
clang/include/clang/Basic/TargetInfo.h
1333

I somehow believe !GridValues is the wrong way around. You should also initialize it directly to null so this makes more sense.

The return value and the type of gridvalues don't match, pick int or unsigned ;)

llvm/include/llvm/Frontend/OpenMP/OMPGridValues.h
109

Nits:
-This is
I don't understand the fixme, do we need that?

saiislam updated this revision to Diff 269519.Jun 9 2020, 6:15 AM
saiislam marked an inline comment as done.
  1. Corrected return type of getGridValue to unsigned
  2. Fixed GridValues assert issue
  3. Edited comments
saiislam marked 2 inline comments as done.Jun 9 2020, 6:16 AM

I am fine with this. @arsenm feel free to accept.

---

Ping.

Afaik, pings should be reserved to ~1 week of inactivity. I know this can be frustrating, but we also need to consider noise for reviewers.

arsenm accepted this revision.Jun 10 2020, 10:32 AM
This revision is now accepted and ready to land.Jun 10 2020, 10:32 AM

I am fine with this. @arsenm feel free to accept.

---

Ping.

Afaik, pings should be reserved to ~1 week of inactivity. I know this can be frustrating, but we also need to consider noise for reviewers.

Ok. I will keep in mind next time. Thanks :-)

This revision was automatically updated to reflect the committed changes.
llvm/include/llvm/Frontend/OpenMP/OMPGridValues.h
66

What's the point of warp_size_32? It's always set to 32 and seems redundant with warp_size

68

log2 of a compile time constant would be computed at compile time, we don't need this field

76

As above, this can be computed at compile time, e.g. from that expression