Page MenuHomePhabricator

[OpenMP][NFC] Pass a DeviceTy, not the device number to `target`
ClosedPublic

Authored by jdoerfert on Feb 10 2021, 9:47 AM.

Details

Summary

This unifies the API of target relative to targetUpdateData and
such.

Diff Detail

Event Timeline

jdoerfert created this revision.Feb 10 2021, 9:47 AM
jdoerfert requested review of this revision.Feb 10 2021, 9:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2021, 9:47 AM
Herald added a subscriber: sstefan1. · View Herald Transcript
tianshilei1992 accepted this revision.Feb 10 2021, 9:51 AM

LG. Some nits.

openmp/libomptarget/src/omptarget.cpp
18

A bit of noise here

openmp/libomptarget/src/private.h
16

We could probably use forward declaration to avoid an include.

This revision is now accepted and ready to land.Feb 10 2021, 9:51 AM
jdoerfert updated this revision to Diff 322721.Feb 10 2021, 9:54 AM

Remove include

Thanks for this. A general trend towards passing structs instead of int and void* is a good thing.

openmp/libomptarget/src/omptarget.cpp
18

^looks like an overenthusiastic editor. stdint.h?

1244

Changes from int64 to int32 here. Either is presumably fine, probably good to use the same width across the library.

openmp/libomptarget/src/private.h
16

Yes, but let's optimise for the compiler finding our mistakes (via include) instead of for compile time for now.

jdoerfert added inline comments.Feb 10 2021, 9:57 AM
openmp/libomptarget/src/omptarget.cpp
18

already gone

1244

int32 is the type of the DeviceID member ...

grokos accepted this revision.Feb 10 2021, 4:33 PM
grokos added inline comments.
openmp/libomptarget/src/omptarget.cpp
1244

Originally, the device ID argument of __tgt_* functions was a 32-bit integer. At some point it was extended to 64 bits in order to store more information like sub-device ID etc.. Internally, however, the library still uses the 32-bit representation. This is not an issue right now because none of the extra information is generated by clang or used by libomptarget yet, but certainly something to watch out for in the future. Nothing to be addressed in the scope of this patch though.

jdoerfert added inline comments.Feb 10 2021, 4:42 PM
openmp/libomptarget/src/omptarget.cpp
1244

We have quite a few of those mismatches we can clean up as we go. As u said, shouldn't break anything right now.

This revision was landed with ongoing or failed builds.Feb 16 2021, 1:38 PM
This revision was automatically updated to reflect the committed changes.