[Openmp-commits] [PATCH] D157918: [Libomptarget] Support mapping indirect host calls to device functions

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added inline comments.


================
Comment at: openmp/libomptarget/DeviceRTL/src/Misc.cpp:86
+    return nullptr;
+
+  uint32_t Left = 0;
----------------
jhuber6 wrote:
> jdoerfert wrote:
> > Check if the HstPtr is outside the table range. This should allows us to avoid the binary search for basically all host pointers in practice.
> We'd need to look up the beginning and the end right? That's still two loads to check.
Yes, up to two loads/checks.
Below you check log(N) + 1 values. 
With Size >= 3 it's a win, assuming device ptrs.


================
Comment at: openmp/libomptarget/include/omptarget.h:95
+  /// Mark the entry global as being an indirectly callable function.
+  OMP_DECLARE_TARGET_INDIRECT = 0x04
 };
----------------
jhuber6 wrote:
> jdoerfert wrote:
> > RaviNarayanaswamy wrote:
> > > jhuber6 wrote:
> > > > RaviNarayanaswamy wrote:
> > > > > Should this be 0x08
> > > > I didn't design it this way, but the frontend uses different flags for globals and kernels. All the other flags overlap so why should this one be the exception without breaking ABI and redesigning it?
> > > You want the lookup table to be  small.  Does TARGET_DTOR get added to the indirect lookup  table?
> > 0x08 plz
> This comes from the previous patch, Clang for whatever reason uses a different set of enums for kernels and globals. All the others overlap so I don't know why this should be the exception. The case where this was important was the registration of the global constructors and destructors, which shouldn't have been looking at globals anyway.
> All the others overlap so I don't know why this should be the exception

I don't see this as a valid argument. If we started fresh, we would not make them overlap to help debugging efforts. Why would one "stick to it" for no good reason?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157918/new/

https://reviews.llvm.org/D157918