Page MenuHomePhabricator

Use named constant to indicate all lanes, to handle 32 and 64 wide architectures [NFC]
ClosedPublic

Authored by JonChesterfield on Oct 2 2019, 6:40 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

JonChesterfield created this revision.Oct 2 2019, 6:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 2 2019, 6:40 PM
grokos added a comment.Oct 2 2019, 6:51 PM

Could alternatively add a constant to target_impl if preferred.

Yes, can you do that instead? No one likes magic numbers :)

  • Replace -1 with named constant
  • add missing chunk
  • add missing chunk
ronlieb added a subscriber: ronlieb.Oct 2 2019, 7:25 PM
ronlieb added inline comments.
openmp/libomptarget/deviceRTLs/nvptx/src/parallel.cu
323 ↗(On Diff #222957)

is there some applicable naming convention for named constants?

Done. Apologies for the multiple revisions & associated emails.

JonChesterfield marked an inline comment as done.Oct 2 2019, 7:29 PM
JonChesterfield added inline comments.
openmp/libomptarget/deviceRTLs/nvptx/src/parallel.cu
323 ↗(On Diff #222957)

Probably, but I'm not sure what. These files are a mixture of the LLVM convention and another one. At some point I guess we should batch convert all the symbols we control to the LLVM convention.

I think a strict reading of the guidelines would be __KmpcImplAllLanes, which I struggle to read. Or possibly __KMPCImplAllLanes. Overall I quite like -1.

grokos added a comment.Oct 2 2019, 7:37 PM

I'm wondering whether it would be better to define __kmpc_impl_all_lanes conditionally, i.e. as 0xFFFF for 32-wide and 0xFFFFFFFF for 64-wide architectures. I'm not sure I like this implicit signed-to-unsigned conversion here... Also, __kmpc_impl_lanemask_t is currently typedef'ed as uint32_t, if we want to support 64-wide lanes, then __kmpc_impl_lanemask_t must also be conditionally defined accordingly.

I'm wondering whether it would be better to define __kmpc_impl_all_lanes conditionally, i.e. as 0xFFFF for 32-wide and 0xFFFFFFFF for 64-wide architectures. I'm not sure I like this implicit signed-to-unsigned conversion here... Also, __kmpc_impl_lanemask_t is currently typedef'ed as uint32_t, if we want to support 64-wide lanes, then __kmpc_impl_lanemask_t must also be conditionally defined accordingly.

More F's. But sure, I'm happy to define it as UINT32_C(0xffffffff) for nvptx instead.

The 64 lane architecture I'm thinking of has it's own target_impl.h where the corresponding typedef is indeed to uint64_t, and all the functions in this file have different implementations. See https://github.com/ROCm-Developer-Tools/llvm-project/blob/AOMP-190918/openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.h if you're curious.

  • Initialise without integer conversion

I'm often against variables in favor of functions but a static const one in a header with guards seems fine to me.

A function has the advantage that it's easier to pick a consistent name for it.

Do we think this is a reasonable change as-is? I don't mind what the variable is called so please speak up if you have a preference.

I'm fine wit this change and the name. I'm happy to accept but I thought people that did participate in the review might want to.

grokos added a comment.Oct 4 2019, 1:41 PM

I'm fine with the changes too. Maybe we should change the title because eventually this patch does not use -1 but rather the full representation (0xffff....).

JonChesterfield retitled this revision from Use -1 to indicate all lanes, to handle 32 and 64 wide architectures to Use named constant to indicate all lanes, to handle 32 and 64 wide architectures.Oct 4 2019, 2:30 PM
JonChesterfield edited the summary of this revision. (Show Details)

Good call on the title change. Thanks, will commit.

grokos accepted this revision.Oct 4 2019, 2:34 PM
This revision is now accepted and ready to land.Oct 4 2019, 2:34 PM
grokos added a comment.Oct 4 2019, 2:36 PM

I also feel the patch must be marked as NFC, there is no functional change here.

JonChesterfield edited the summary of this revision. (Show Details)Oct 4 2019, 2:39 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 4 2019, 2:40 PM
grokos retitled this revision from Use named constant to indicate all lanes, to handle 32 and 64 wide architectures to Use named constant to indicate all lanes, to handle 32 and 64 wide architectures [NFC].Oct 4 2019, 2:42 PM

Thanks for the NFC recommendation. I'll tag with that where appropriate from now on.