Page MenuHomePhabricator

[OpenMP][NFCI] Cleanup APIs and improve object encapsulation
AcceptedPublic

Authored by jdoerfert on Apr 8 2022, 10:14 PM.

Details

Summary

As a preparation patch this one cleans up the API for getTargetPointer
and getTgtPtrBegin. It also encapsulates TargetPointerResultTy properly.
The latter will become more important as we lock the entries
consistently. The former helps us to avoid inconsistencies, e.g., we
returned IsHostPointer in the Flags of the TargetPointerResultTy but
also via the boolean reference. This patch eliminates both reference
arguments.

Diff Detail

Event Timeline

jdoerfert created this revision.Apr 8 2022, 10:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2022, 10:14 PM
jdoerfert requested review of this revision.Apr 8 2022, 10:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2022, 10:14 PM
Herald added a subscriber: sstefan1. · View Herald Transcript
ye-luo added inline comments.Apr 11 2022, 1:48 PM
openmp/libomptarget/include/device.h
285–288

three 0?

openmp/libomptarget/src/device.cpp
241

Can we avoid all the setXXX functions?
I feel better keep the local IsNew and then return by calling TPR::TPR(Entry, TargetPointer, IsNew...)

jdoerfert added inline comments.Apr 11 2022, 4:02 PM
openmp/libomptarget/src/device.cpp
241

But then I need to do manual locking again and I would like us to avoid that.

ye-luo added inline comments.Apr 11 2022, 4:59 PM
openmp/libomptarget/src/device.cpp
241

Do you have a patch with locking added? Try to understand better your intention.

jdoerfert added inline comments.Apr 11 2022, 8:06 PM
openmp/libomptarget/src/device.cpp
241

I do not. Every setEntry will lock the entry and unlock the lat entry, if any. Every time TPR goes out of scope, e.g., when we give up and return an dummy object, you unlock the entry. If you do manual locking the caller needs to also unlock when TPR is destroyed, various places all across the code base.

ye-luo added inline comments.Apr 11 2022, 8:26 PM
openmp/libomptarget/src/device.cpp
241

Assuming the lock is in the entry, it makes sense.

ye-luo accepted this revision.Apr 12 2022, 7:05 AM
This revision is now accepted and ready to land.Apr 12 2022, 7:05 AM
jdoerfert added inline comments.Apr 12 2022, 8:05 AM
openmp/libomptarget/src/device.cpp
241

Yes. Entry is the one with the lock. TPR is bacically a exclusive accessor with some extra information. @tianshilei1992 asked for more documentation, I will add that before I merge.

jdoerfert added inline comments.Apr 14 2022, 1:04 PM
openmp/libomptarget/src/device.cpp
391

This belongs into the next patch of this series.

Address comments

Fix rebase errors, address comments