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

[Artem Belevich via Phabricator via llvm-commits]

tra added a comment.

LGTM in general, modulo the comments below.



================
Comment at: llvm/include/llvm/IR/IntrinsicsNVVM.td:657
-      Intrinsic<[llvm_float_ty], [llvm_float_ty, llvm_float_ty],
+      DefaultAttrsIntrinsic<[llvm_float_ty], [llvm_float_ty, llvm_float_ty],
         [IntrNoMem, Commutative]>;
   def int_nvvm_div_approx_f : GCCBuiltin<"__nvvm_div_approx_f">,
----------------
jdoerfert wrote:
> These commutative annotations seem wrong to me. Same for `fma`.
Agreed. 


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


================
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]>;
----------------
Should this be speculatable, too? It just bitcasts two ints to a double. There are no side effects.


================
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]>;
 
----------------
Some of the special registers provide clock and performance monitoring counter info. Speculating those may not be a good idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109987