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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
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">,
----------------
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.


================
Comment at: llvm/include/llvm/IR/IntrinsicsNVVM.td:1089
   def int_nvvm_lohi_i2d : GCCBuiltin<"__nvvm_lohi_i2d">,
-      Intrinsic<[llvm_double_ty], [llvm_i32_ty, llvm_i32_ty],
+      DefaultAttrsIntrinsic<[llvm_double_ty], [llvm_i32_ty, llvm_i32_ty],
         [IntrNoMem, Commutative]>;
----------------
tra wrote:
> Should this be speculatable, too? It just bitcasts two ints to a double. There are no side effects.
It escaped my search pattern due to the commutative. Will add speculatable.


================
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]>;
 
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109987