[PATCH] D112718: Add intrinsics and builtins for PTX atomics with semantic orders

[Artem Belevich via Phabricator via llvm-commits]

tra added inline comments.


================
Comment at: clang/include/clang/Basic/BuiltinsNVPTX.def:1057
+
+BUILTIN(__nvvm_atom_xchg_global_i, "iiD*i", "n")
+TARGET_BUILTIN(__nvvm_atom_cta_xchg_global_i, "iiD*i", "n", SM_60)
----------------
t4c1 wrote:
> tra wrote:
> > We need to figure out how address-space-specific builtins are supposed to work.
> > Right now two competing approaches.
> > 
> > This patch declares builtins with generic pointer as an argument, but, according to the test, expects to be used with the AS-specific pointer.
> > It probably does not catch a wrong-AS pointer passed as an argument, either.
> > It does happen to work, but I think it's mostly due to the fact that LLVM intrinsics are overloaded and we happen to end up addrspacecast'ing  things correctly if the builtin gets the right kind of pointer.
> > 
> > The other approach is to declare the pointer with the expected AS. E.g:
> > > TARGET_BUILTIN(__nvvm_mbarrier_init_shared, "vWi*3i", "", AND(SM_80,PTX70))
> > 
> > IMO, this is the correct way to do it, but it is also rather inconvenient to use from CUDA code as it operates on generic pointers.
> > 
> > @jdoerfert - WDYT?
> >TARGET_BUILTIN(__nvvm_mbarrier_init_shared, "vWi*3i", "", AND(SM_80,PTX70))
> >IMO, this is the correct way to do it, but it is also rather inconvenient to use from CUDA code as it operates on generic pointers.
> 
> I tried doing this, however it is also completely unusable from OpenCL code (which is our use case). Trying to use it gives you errors about how casting pointers to different address spaces  - for example from local to AS3 is not allowed.
Hmm. It should've worked. It would need the same explicit cast to a pointer in AS(3) as in your tests, but it would safeguard against attempts to pass it a generic pointer. E.g. https://godbolt.org/z/qE6oxzheM

Explicit casting to AS(X) for AS-specific variants is annoying, but it's probably unavoidable at the moment regardless of whether we declare the pointer argument to be AS-specific or not.  LLVM will not always be able to infer that a pointer is in particular AS.
Using specific AS in the declaration has a minor benefit of safeguarding at compile time against unintentional use of generic pointers.

Ideally we may want to convert generic variant of the builtin to AS-specific one, if LLVM does know the AS. We currently do this for loads/stores, but not for other instructions.



================
Comment at: clang/test/CodeGen/builtins-nvptx.c:557
+  // expected-error at +1 {{'__nvvm_atom_acquire_add_global_i' needs target feature sm_70}}
+  __nvvm_atom_acquire_add_global_i((__attribute__((address_space(1))) int *)ip, i);
+
----------------
t4c1 wrote:
> tra wrote:
> > What happens if I pass a wrong pointer kind? E.g. a generic or shared pointer?
> It will silently accept it. I can look into how to output appropriate error message.
I guest the bast we can do here is to safeguard against unintentional use of generic pointer.
There's not much we can do if someone explicitly casts a pointer to a wrong AS.

I think declaring pointer arg to be in specific AS would be sufficient. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112718