This is an archive of the discontinued LLVM Phabricator instance.

[openmp][nfc] Refactor GridValues
ClosedPublic

Authored by JonChesterfield on Aug 19 2021, 8:34 AM.

Details

Summary

Remove redundant fields and replace pointer with virtual function

Of fourteen fields, three are dead and four can be computed from the
remainder. This leaves a couple of currently dead fields in place as
they are expected to be used from the deviceRTL shortly. Two of the
fields that can be computed are only used from codegen and require a
log2() implementation so are inlined into codegen instead.

This change leaves the new methods in the same location in the struct
as the previous fields for convenience at review.

Diff Detail

Event Timeline

JonChesterfield requested review of this revision.Aug 19 2021, 8:34 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 19 2021, 8:34 AM
  • reorder field
JonChesterfield edited the summary of this revision. (Show Details)Aug 19 2021, 10:09 AM
jdoerfert added inline comments.Aug 19 2021, 10:58 AM
llvm/include/llvm/Frontend/OpenMP/OMPGridValues.h
95

It should be in the device rtl then, no?

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

This header is currently used from clang and the (amdgpu, could also be cuda if we like) host plugin. Possibly also from llvm. As of D108391 it would be used from the devicertl.

The idea is to have a single source of truth for the various magic numbers that the pieces should agree on and llvm is the common point on the dependency tree. I'm currently interested in that because I want to change some of them for gfx10 and have that magically ripple through the components. I'm not totally confident that will work out nicely for the host plugin as it has to dynamically handle different architectures but I think it'll be good enough.

It's not totally ideal to hand spin a function that is in the math support header but I also don't want to try to make various llvm headers ffreestanding-safe.

jdoerfert added inline comments.Aug 19 2021, 11:44 AM
llvm/include/llvm/Frontend/OpenMP/OMPGridValues.h
95

It's not totally ideal to hand spin a function that is in the math support header but I also don't want to try to make various llvm headers ffreestanding-safe.

The function is only needed in the device rtl. Put it in the device rtl.

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

This particular function is only called by warpSizeLog2 which is currently only used by CGOpenMPRuntimeGPU. The deviceRTL doesn't call the function. However if this header includes the rest of llvm support then it can't call any of the others either.

I'm going to drop the log2 accessor (and this function) in favour of two calls into math support from CGOpenMPRuntime.

  • delete log2 accessors per review comments
JonChesterfield edited the summary of this revision. (Show Details)Aug 19 2021, 12:19 PM
  • whitespace, drop asserts
JonChesterfield edited the summary of this revision. (Show Details)Aug 19 2021, 12:26 PM
This revision is now accepted and ready to land.Aug 20 2021, 8:11 AM
This revision was landed with ongoing or failed builds.Aug 20 2021, 8:41 AM
This revision was automatically updated to reflect the committed changes.

Failed a nvptx codegen test (maybe the change to calculate log2 at runtime), currently away from my desk but will revert when I get back unless beaten to it.

JonChesterfield reopened this revision.Aug 20 2021, 10:18 AM
This revision is now accepted and ready to land.Aug 20 2021, 10:18 AM
clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
551

Bug is here. ~0 >> 27u == -1 (bad) and ~0u >> 27u == 31 (good). Win for exact codegen tests.

  • require unsigned shift
This revision was automatically updated to reflect the committed changes.