[PATCH] D109987: [NVVM] Update intrinsic definitions to include more attributes

[Artem Belevich via Phabricator via llvm-commits]

tra accepted this revision.
tra added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/include/llvm/IR/IntrinsicsNVVM.td:817
+      DefaultAttrsIntrinsic<[llvm_float_ty], [llvm_float_ty, llvm_float_ty, llvm_float_ty],
+        [IntrNoMem, IntrSpeculatable, Commutative]>;
   def int_nvvm_fma_rn_f : GCCBuiltin<"__nvvm_fma_rn_f">,
----------------
jdoerfert wrote:
> tra wrote:
> > I'm not sure what `Commutative` attribute would mean for FMA. I guess the multiplicand args are commutative with each other, but the addend is not. I guess the Commutative should ideally list the operands it applies to.
> I'm for removing it here and above.
SGTM.


================
Comment at: llvm/include/llvm/IR/IntrinsicsNVVM.td:4272
 // FIXME: Do we need the 128-bit integer type version?
-//    def _r64   : Intrinsic<[llvm_i128_ty],   [], [IntrNoMem]>;
+//    def _r64   : Intrinsic<[llvm_i128_ty],   [], [IntrNoMem, IntrSpeculatable]>;
 
----------------
jdoerfert wrote:
> tra wrote:
> > Some of the special registers provide clock and performance monitoring counter info. Speculating those may not be a good idea.
> As far as I can tell, those are the `PTXReadNCSRegIntrinsic_r32` ones below. If any of the IntrNoMem ones would do this, we would CSE them and their usefulness would be rather limited. As long as we allow CSE, I think, all we care about is the potential for UB in case we speculate on them. Reading registers hopefully won't cause UB.
You're right. These are fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109987