[PATCH] D58353: SystemZ: Add ImmArg to intrinsics

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

Thanks, this should now cover all intrinsics with immediate arguments.  Some additional comments inline (mostly cosmetic, with one real fix for int_s390_vfisb).



================
Comment at: include/llvm/IR/IntrinsicsSystemZ.td:47
 
-class SystemZTernaryConv<string name, LLVMType result, LLVMType arg>
+class SystemZTernaryConv<string name, LLVMType result, LLVMType arg, list<IntrinsicProperty> intr_properties = []>
   : GCCBuiltin<"__builtin_s390_" ## name>,
----------------
There should be no need to change this.


================
Comment at: include/llvm/IR/IntrinsicsSystemZ.td:52
+class SystemZTernary<string name, LLVMType type, list<IntrinsicProperty> intr_properties = []>
+  : SystemZTernaryConv<name, type, type, intr_properties>;
 
----------------
Or this.


================
Comment at: include/llvm/IR/IntrinsicsSystemZ.td:56
   : GCCBuiltin<"__builtin_s390_" ## name>,
-    Intrinsic<[type], [type, type, llvm_i32_ty], [IntrNoMem]>;
+    Intrinsic<[type], [type, type, llvm_i32_ty], !listconcat([IntrNoMem], intr_properties)>;
 
----------------
Just hard-code ImmArg<2> here, it will be needed by all users.


================
Comment at: include/llvm/IR/IntrinsicsSystemZ.td:66
+    Intrinsic<[type], [type, type, type, llvm_i32_ty],
+    !listconcat([IntrNoMem], intr_properties)>;
 
----------------
Likewise just hard-code ImmArg<3>.


================
Comment at: include/llvm/IR/IntrinsicsSystemZ.td:168
 
-multiclass SystemZTernaryIntBHF<string name> {
-  def b : SystemZTernaryInt<name##"b", llvm_v16i8_ty>;
-  def h : SystemZTernaryInt<name##"h", llvm_v8i16_ty>;
-  def f : SystemZTernaryInt<name##"f", llvm_v4i32_ty>;
+multiclass SystemZTernaryIntBHF<string name, list<IntrinsicProperty> intr_properties = []> {
+  def b : SystemZTernaryInt<name##"b", llvm_v16i8_ty, intr_properties>;
----------------
This change can then go away.


================
Comment at: include/llvm/IR/IntrinsicsSystemZ.td:180
 
-multiclass SystemZQuaternaryIntBHF<string name> {
-  def b : SystemZQuaternaryInt<name##"b", llvm_v16i8_ty>;
-  def h : SystemZQuaternaryInt<name##"h", llvm_v8i16_ty>;
-  def f : SystemZQuaternaryInt<name##"f", llvm_v4i32_ty>;
+multiclass SystemZQuaternaryIntBHF<string name, list<IntrinsicProperty> intr_properties = []> {
+  def b : SystemZQuaternaryInt<name##"b", llvm_v16i8_ty, intr_properties>;
----------------
And this.


================
Comment at: include/llvm/IR/IntrinsicsSystemZ.td:186
 
-multiclass SystemZQuaternaryIntBHFG<string name> : SystemZQuaternaryIntBHF<name> {
-  def g : SystemZQuaternaryInt<name##"g", llvm_v2i64_ty>;
+multiclass SystemZQuaternaryIntBHFG<string name,
+           list<IntrinsicProperty> intr_properties = []> :
----------------
And this too.


================
Comment at: include/llvm/IR/IntrinsicsSystemZ.td:307
   defm int_s390_verll  : SystemZBinaryIntBHFG<"verll">;
-  defm int_s390_verim  : SystemZQuaternaryIntBHFG<"verim">;
+  defm int_s390_verim  : SystemZQuaternaryIntBHFG<"verim", [ImmArg<3>]>;
 
----------------
This would be covered by the default.


================
Comment at: include/llvm/IR/IntrinsicsSystemZ.td:347
 
-  defm int_s390_vfae  : SystemZTernaryIntBHF<"vfae">;
+  defm int_s390_vfae  : SystemZTernaryIntBHF<"vfae", [ImmArg<2>]>;
   defm int_s390_vfae  : SystemZTernaryIntCCBHF;
----------------
And those two.


================
Comment at: include/llvm/IR/IntrinsicsSystemZ.td:365
 
-  defm int_s390_vstrc  : SystemZQuaternaryIntBHF<"vstrc">;
+  defm int_s390_vstrc  : SystemZQuaternaryIntBHF<"vstrc", [ImmArg<3>]>;
   defm int_s390_vstrc  : SystemZQuaternaryIntCCBHF;
----------------
Likewise.


================
Comment at: include/llvm/IR/IntrinsicsSystemZ.td:410
                                  [llvm_v4f32_ty, llvm_i32_ty, llvm_i32_ty],
-                                 [IntrNoMem]>;
+                                 [IntrNoMem, ImmArg<2>]>;
 
----------------
This should have both ImmArg<1> and ImmArg<2>.


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

https://reviews.llvm.org/D58353