[PATCH] D125639: [NVPTX] Enable AtomicExpandPass for NVPTX

[Artem Belevich via Phabricator via llvm-commits]

tra added inline comments.


================
Comment at: llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp:5160
+    default:
+      return AtomicExpansionKind::CmpXChg;
+    }
----------------
tianshilei1992 wrote:
> tra wrote:
> > tianshilei1992 wrote:
> > > tra wrote:
> > > > Can we handle types other than 32 and 64-bit? If yes, we need tests at least for some of them (i8/i16/f16/i128). If not, this should probably be `llvm_unreachable()`.
> > > This is quite interesting. I don't think CUDA can generate that kind of code, but OpenMP can. However, for other types, like `i16`, with the atomic expand, it's just converted to `AtomicCmpSwap` of type `i16`, and then the isel crashed. For now, I guess the best way is to use `llvm_unreachable` for other cases. In the long term, we may want to support the expansion of types with different bitwidth, but I don't have a clear idea how that would work. Otherwise, we will have to tell the front end that some types are not supported, which kind of breaks the assumption that front end can emit target agnostic code.
> > > I don't think CUDA can generate that kind of code,
> > 
> > Why do you believe that's the case? I would assume that if a C++ compilation can, so would CUDA, though we may be sort of lucky and may currently not have appropriate GPU-side overloads for `std::atomic` functions. We do want to make it work, so it's likely to get fixed sooner or later.
> > 
> > It may still be a good idea to add these extra test cases to the test, too, possibly commented out with a comment explaining what's going on.
> > 
> I added tests for `i8` and `i16`. Another types are not covered because based on https://docs.nvidia.com/cuda/nvvm-ir-spec/index.html#type-system they are not supported at the IR level.
> they are not supported [by NVVM] at the IR level.

NVVM != LLVM, so it's not a particularly strong reason.

LLVM does have limited support for i128 and we should cover it, even if the answer is "it does not work (yet?)".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125639