This is an archive of the discontinued LLVM Phabricator instance.

[openmp][amdgpu][nfc] Simplify implicit args handling
ClosedPublic

Authored by JonChesterfield on Nov 19 2021, 11:43 AM.

Details

Summary

Removes a +x/-x pair on the only store/load of a variable
and deletes some nearby dead code. Also reduces the size of the implicit
struct to reflect the code currently emitted by clang.

Diff Detail

Event Timeline

JonChesterfield requested review of this revision.Nov 19 2021, 11:43 AM
Herald added a project: Restricted Project. · View Herald Transcript
openmp/libomptarget/plugins/amdgpu/impl/system.cpp
488

info.kernel_segment_size is only written here

openmp/libomptarget/plugins/amdgpu/src/rtl.cpp
1634

info.kernel_segment_size is only read here

arsenm added inline comments.Nov 19 2021, 11:48 AM
openmp/libomptarget/plugins/amdgpu/impl/system.cpp
487–488

The naming here is going to be confusing. The compiler reported kernarg size is supposed to be the complete allocation size. If this wants to know the explicit argument size, that should probably go to a different named field

arsenm added inline comments.Nov 19 2021, 11:50 AM
openmp/libomptarget/plugins/amdgpu/impl/system.cpp
487–488

i.e. nothing about checking hasHiddenArgs makes sense

openmp/libomptarget/plugins/amdgpu/impl/system.cpp
487–488

It's less confusing after this patch than it was before. Incremental steps. I'm deleting stuff while I work out how this handling is behaving.

JonChesterfield edited the summary of this revision. (Show Details)Nov 19 2021, 12:18 PM
This revision was not accepted when it landed; it landed in state Needs Review.Nov 19 2021, 12:18 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
brooks added a subscriber: brooks.Jan 10 2022, 10:21 AM
brooks added inline comments.
openmp/libomptarget/plugins/amdgpu/impl/internal.h
45

This breaks builds on 32-bit platforms (e.g., i386). If they aren't supported that's fine, but then cmake adjustments are required to prevent it from trying to build.

arsenm added inline comments.Jan 10 2022, 10:24 AM
openmp/libomptarget/plugins/amdgpu/impl/internal.h
45

Just use explicit uint64_t, the named types randomly changing sizes is one of the dumbest things about C

openmp/libomptarget/plugins/amdgpu/impl/internal.h
45

I don't know if i386 will work in general, but those ^ have to be 64 bit regardless, will fix shortly

openmp/libomptarget/plugins/amdgpu/impl/internal.h
45

Patch at D116963