[Openmp-commits] [openmp] ee1bf45 - [OpenMP][NFC] Added DeviceID and Event pointer to __tgt_async_info

[Jonas Hahnfeld via Openmp-commits]

The comment says "make async info a pointer argument in a separate
(NFC) patch" which is, in my opinion, completely unrelated to changing
the layout of the data structure. Please see the LLVM lexicon for the
meaning of "NFC": https://llvm.org/docs/Lexicon.html?highlight=nfc#n

I think this patch should be reverted because it provides no value
being separated from the patch that uses it. I agree with Johannes on
passing the pointer argument around, but not with this patch.

Jonas

Am Mittwoch, den 17.06.2020, 14:44 -0400 schrieb Shilei Tian:
> It is NFC because they’re not used actually, and going to be used in a large patch, which we think might be better to be splitted into some smaller ones.
> 
> https://reviews.llvm.org/D81989#inline-753797
> 
> Regards,
> Shilei
> 
> > On Jun 17, 2020, at 14:39, Jonas Hahnfeld <hahnjo at hahnjo.de> wrote:
> > 
> > Am Mittwoch, den 17.06.2020, 11:31 -0700 schrieb Shilei Tian via
> > Openmp-commits:
> > > Author: Shilei Tian
> > > Date: 2020-06-17T14:29:09-04:00
> > > New Revision: ee1bf45e1d42d7f386d8321c3a8799476344ad91
> > > 
> > > URL: 
> > > https://github.com/llvm/llvm-project/commit/ee1bf45e1d42d7f386d8321c3a8799476344ad91
> > > 
> > > DIFF: 
> > > https://github.com/llvm/llvm-project/commit/ee1bf45e1d42d7f386d8321c3a8799476344ad91.diff
> > > 
> > > 
> > > LOG: [OpenMP][NFC] Added DeviceID and Event pointer to __tgt_async_info
> > > 
> > > DeviceID is added for some cases that we only have the __tgt_async_info but do
> > > not know its corresponding device id. However, to communicate with target
> > > plugins, we need that information.
> > > 
> > > Event is added for another way to synchronize.
> > > 
> > > Added: 
> > > 
> > > 
> > > Modified: 
> > >    openmp/libomptarget/include/omptarget.h
> > > 
> > > Removed: 
> > > 
> > > 
> > > 
> > > ################################################################################
> > > diff  --git a/openmp/libomptarget/include/omptarget.h b/openmp/libomptarget/include/omptarget.h
> > > index de3afc36c7f2..7ba088c1996b 100644
> > > --- a/openmp/libomptarget/include/omptarget.h
> > > +++ b/openmp/libomptarget/include/omptarget.h
> > > @@ -114,10 +114,14 @@ struct __tgt_target_table {
> > > /// This struct contains information exchanged between 
> > > diff erent asynchronous
> > > /// operations for device-dependent optimization and potential synchronization
> > > struct __tgt_async_info {
> > > +  // Device ID. Note that it is NOT the RTLDeviceID. We don't need to store the
> > > +  // RTLDeviceID explicitly as we can always get it via DeviceID.
> > > +  int DeviceID = -1;
> > >   // A pointer to a queue-like structure where offloading operations are issued.
> > > -  // We assume to use this structure to do synchronization. In CUDA backend, it
> > > -  // is CUstream.
> > > +  // We assume to use this structure to do synchronization.
> > >   void *Queue = nullptr;
> > > +  // A pointer to a device-dependent event used for synchronization as well.
> > > +  void *Event = nullptr;
> > > };
> > 
> > Adding new members to a data structure is definitely _not_ NFC.
> > Moreover, the new fields are currently unused because this commit is
> > only changing the data structure in the header. Was this patch reviewed
> > somewhere?
> > 
> > Jonas
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <http://lists.llvm.org/pipermail/openmp-commits/attachments/20200617/778fccfa/attachment-0001.sig>