SystemZ: Add ImmArg to intrinsics

[arsenm]

Summary

I found these by asserting in clang for any GCCBuiltin that doesn't
require mangling and requires a constant for the builtin. This means
that intrinsics are missing which don't use GCCBuiltin, don't have
builtins defined in clang, or were missing the constant annotation in
the builtin definition.

[uweigand]

I'm not quite sure I understand the logic why some intrinsics that require immediate arguments are marked with ImmArg, but others are not?

Shouldn't we mark all of them? The ones I see missing in your patch are:

defm int_s390_vfae  : SystemZTernaryIntCCBHF;
defm int_s390_vfaez : SystemZTernaryIntCCBHF;
defm int_s390_vstrc  : SystemZQuaternaryIntCCBHF;
defm int_s390_vstrcz : SystemZQuaternaryIntCCBHF;
def int_s390_vfmaxdb : Intrinsic<[llvm_v2f64_ty],
def int_s390_vfmindb : Intrinsic<[llvm_v2f64_ty],
def int_s390_vfmaxsb : Intrinsic<[llvm_v4f32_ty],
def int_s390_vfminsb : Intrinsic<[llvm_v4f32_ty],
def int_s390_vftcidb : SystemZBinaryConvIntCC<llvm_v2i64_ty, llvm_v2f64_ty>;
def int_s390_vftcisb : SystemZBinaryConvIntCC<llvm_v4i32_ty, llvm_v4f32_ty>;
def int_s390_vfidb : Intrinsic<[llvm_v2f64_ty],
def int_s390_vfisb : Intrinsic<[llvm_v4f32_ty],

Maybe it would actually make more sense to add the ImmArg directly in the SystemZ*Int* helper macros; those intrinsics really all need immediate arguments. (The sole exception I can see is verll, but that probably should use a different helper then.)

[arsenm]

In D58353#1401312, @uweigand wrote:

I'm not quite sure I understand the logic why some intrinsics that require immediate arguments are marked with ImmArg, but others are not?

Shouldn't we mark all of them? The ones I see missing in your patch are:

defm int_s390_vfae  : SystemZTernaryIntCCBHF;
defm int_s390_vfaez : SystemZTernaryIntCCBHF;
defm int_s390_vstrc  : SystemZQuaternaryIntCCBHF;
defm int_s390_vstrcz : SystemZQuaternaryIntCCBHF;
def int_s390_vfmaxdb : Intrinsic<[llvm_v2f64_ty],
def int_s390_vfmindb : Intrinsic<[llvm_v2f64_ty],
def int_s390_vfmaxsb : Intrinsic<[llvm_v4f32_ty],
def int_s390_vfminsb : Intrinsic<[llvm_v4f32_ty],
def int_s390_vftcidb : SystemZBinaryConvIntCC<llvm_v2i64_ty, llvm_v2f64_ty>;
def int_s390_vftcisb : SystemZBinaryConvIntCC<llvm_v4i32_ty, llvm_v4f32_ty>;
def int_s390_vfidb : Intrinsic<[llvm_v2f64_ty],
def int_s390_vfisb : Intrinsic<[llvm_v4f32_ty],

Maybe it would actually make more sense to add the ImmArg directly in the SystemZ*Int* helper macros; those intrinsics really all need immediate arguments. (The sole exception I can see is verll, but that probably should use a different helper then.)

I'm doing this blindly based on the definitions here https://github.com/llvm-mirror/clang/blob/master/include/clang/Basic/BuiltinsSystemZ.def
Are these accurate and complete?

[uweigand]

In D58353#1401325, @arsenm wrote:

In D58353#1401312, @uweigand wrote:

I'm not quite sure I understand the logic why some intrinsics that require immediate arguments are marked with ImmArg, but others are not?

Shouldn't we mark all of them? The ones I see missing in your patch are:

defm int_s390_vfae  : SystemZTernaryIntCCBHF;
defm int_s390_vfaez : SystemZTernaryIntCCBHF;
defm int_s390_vstrc  : SystemZQuaternaryIntCCBHF;
defm int_s390_vstrcz : SystemZQuaternaryIntCCBHF;
def int_s390_vfmaxdb : Intrinsic<[llvm_v2f64_ty],
def int_s390_vfmindb : Intrinsic<[llvm_v2f64_ty],
def int_s390_vfmaxsb : Intrinsic<[llvm_v4f32_ty],
def int_s390_vfminsb : Intrinsic<[llvm_v4f32_ty],
def int_s390_vftcidb : SystemZBinaryConvIntCC<llvm_v2i64_ty, llvm_v2f64_ty>;
def int_s390_vftcisb : SystemZBinaryConvIntCC<llvm_v4i32_ty, llvm_v4f32_ty>;
def int_s390_vfidb : Intrinsic<[llvm_v2f64_ty],
def int_s390_vfisb : Intrinsic<[llvm_v4f32_ty],

Maybe it would actually make more sense to add the ImmArg directly in the SystemZ*Int* helper macros; those intrinsics really all need immediate arguments. (The sole exception I can see is verll, but that probably should use a different helper then.)

I'm doing this blindly based on the definitions here https://github.com/llvm-mirror/clang/blob/master/include/clang/Basic/BuiltinsSystemZ.def
Are these accurate and complete?

Yes, they are ... but note that all the intrinsics I listed above are actually marked as requiring immediates there too. (E.g. int_s390_vfmaxdb gets emitted from __builtin_s390_vfmaxdb, which has an "Ii" marker.)

[arsenm]

In D58353#1401332, @uweigand wrote:

In D58353#1401325, @arsenm wrote:

In D58353#1401312, @uweigand wrote:

I'm not quite sure I understand the logic why some intrinsics that require immediate arguments are marked with ImmArg, but others are not?

Shouldn't we mark all of them? The ones I see missing in your patch are:

defm int_s390_vfae  : SystemZTernaryIntCCBHF;
defm int_s390_vfaez : SystemZTernaryIntCCBHF;
defm int_s390_vstrc  : SystemZQuaternaryIntCCBHF;
defm int_s390_vstrcz : SystemZQuaternaryIntCCBHF;
def int_s390_vfmaxdb : Intrinsic<[llvm_v2f64_ty],
def int_s390_vfmindb : Intrinsic<[llvm_v2f64_ty],
def int_s390_vfmaxsb : Intrinsic<[llvm_v4f32_ty],
def int_s390_vfminsb : Intrinsic<[llvm_v4f32_ty],
def int_s390_vftcidb : SystemZBinaryConvIntCC<llvm_v2i64_ty, llvm_v2f64_ty>;
def int_s390_vftcisb : SystemZBinaryConvIntCC<llvm_v4i32_ty, llvm_v4f32_ty>;
def int_s390_vfidb : Intrinsic<[llvm_v2f64_ty],
def int_s390_vfisb : Intrinsic<[llvm_v4f32_ty],

Maybe it would actually make more sense to add the ImmArg directly in the SystemZ*Int* helper macros; those intrinsics really all need immediate arguments. (The sole exception I can see is verll, but that probably should use a different helper then.)

I'm doing this blindly based on the definitions here https://github.com/llvm-mirror/clang/blob/master/include/clang/Basic/BuiltinsSystemZ.def
Are these accurate and complete?

Yes, they are ... but note that all the intrinsics I listed above are actually marked as requiring immediates there too. (E.g. int_s390_vfmaxdb gets emitted from __builtin_s390_vfmaxdb, which has an "Ii" marker.)

These ones aren't using GCCBuiltin, so that's why they were missed

[arsenm]

Handle missed intrinsics

[uweigand]

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

include/llvm/IR/IntrinsicsSystemZ.td

46

There should be no need to change this.

51

Or this.

55

Just hard-code ImmArg<2> here, it will be needed by all users.

64

Likewise just hard-code ImmArg<3>.

166–169

This change can then go away.

178–181

And this.

184–187

And this too.

305

This would be covered by the default.

345

And those two.

363

Likewise.

408

This should have both ImmArg<1> and ImmArg<2>.

[arsenm]

Address comments

[uweigand]

LGTM, thanks!

[arsenm]

r356091

include/llvm/IR/IntrinsicsSystemZ.td

class SystemZBinaryConvCC<LLVMType result, LLVMType arg>
   : Intrinsic<[result, llvm_i32_ty], [arg, arg], [IntrNoMem]>;
 
 class SystemZBinaryConvIntCC<LLVMType result, LLVMType arg>
   : Intrinsic<[result, llvm_i32_ty], [arg, llvm_i32_ty], [IntrNoMem]>;
 
 class SystemZBinaryCC<LLVMType type>
   : SystemZBinaryConvCC<type, type>;
 
-class SystemZTernaryConv<string name, LLVMType result, LLVMType arg>
+class SystemZTernaryConv<string name, LLVMType result, LLVMType arg, list<IntrinsicProperty> intr_properties = []>
   : GCCBuiltin<"__builtin_s390_" ## name>,
-    Intrinsic<[result], [arg, arg, result], [IntrNoMem]>;
+    Intrinsic<[result], [arg, arg, result], !listconcat([IntrNoMem], intr_properties)>;
 
-class SystemZTernary<string name, LLVMType type>
-  : SystemZTernaryConv<name, type, type>;
+class SystemZTernary<string name, LLVMType type, list<IntrinsicProperty> intr_properties = []>
+  : SystemZTernaryConv<name, type, type, intr_properties>;
 
-class SystemZTernaryInt<string name, LLVMType type>
+class SystemZTernaryInt<string name, LLVMType type, list<IntrinsicProperty> intr_properties = []>
   : GCCBuiltin<"__builtin_s390_" ## name>,
-    Intrinsic<[type], [type, type, llvm_i32_ty], [IntrNoMem]>;
+    Intrinsic<[type], [type, type, llvm_i32_ty], !listconcat([IntrNoMem], intr_properties)>;
 
-class SystemZTernaryIntCC<LLVMType type>
-  : Intrinsic<[type, llvm_i32_ty], [type, type, llvm_i32_ty], [IntrNoMem]>;
+class SystemZTernaryIntCC<LLVMType type, list<IntrinsicProperty> intr_properties = []>
+  : Intrinsic<[type, llvm_i32_ty], [type, type, llvm_i32_ty], !listconcat([IntrNoMem], intr_properties)>;
 
-class SystemZQuaternaryInt<string name, LLVMType type>
+class SystemZQuaternaryInt<string name, LLVMType type,
+                           list<IntrinsicProperty> intr_properties = []>
   : GCCBuiltin<"__builtin_s390_" ## name>,
-    Intrinsic<[type], [type, type, type, llvm_i32_ty], [IntrNoMem]>;
+    Intrinsic<[type], [type, type, type, llvm_i32_ty],
+    !listconcat([IntrNoMem], intr_properties)>;
 
 class SystemZQuaternaryIntCC<LLVMType type>
   : Intrinsic<[type, llvm_i32_ty], [type, type, type, llvm_i32_ty],
               [IntrNoMem]>;
 
 multiclass SystemZUnaryExtBHF<string name> {
   def b : SystemZUnaryConv<name##"b", llvm_v8i16_ty, llvm_v16i8_ty>;
   def h : SystemZUnaryConv<name##"h", llvm_v4i32_ty, llvm_v8i16_ty>;

 }
 
 multiclass SystemZTernaryBHF<string name> {
   def b : SystemZTernary<name##"b", llvm_v16i8_ty>;
   def h : SystemZTernary<name##"h", llvm_v8i16_ty>;
   def f : SystemZTernary<name##"f", llvm_v4i32_ty>;
 }
 
-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>;
+  def h : SystemZTernaryInt<name##"h", llvm_v8i16_ty, intr_properties>;
+  def f : SystemZTernaryInt<name##"f", llvm_v4i32_ty, intr_properties>;
 }
 
 multiclass SystemZTernaryIntCCBHF {
   def bs : SystemZTernaryIntCC<llvm_v16i8_ty>;
   def hs : SystemZTernaryIntCC<llvm_v8i16_ty>;
   def fs : SystemZTernaryIntCC<llvm_v4i32_ty>;
 }
 
-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>;
+  def h : SystemZQuaternaryInt<name##"h", llvm_v8i16_ty, intr_properties>;
+  def f : SystemZQuaternaryInt<name##"f", llvm_v4i32_ty, intr_properties>;
 }
 
-multiclass SystemZQuaternaryIntBHFG<string name> : SystemZQuaternaryIntBHF<name> {
-  def g : SystemZQuaternaryInt<name##"g", llvm_v2i64_ty>;
+multiclass SystemZQuaternaryIntBHFG<string name,
+           list<IntrinsicProperty> intr_properties = []> :
+  SystemZQuaternaryIntBHF<name, intr_properties> {
+  def g : SystemZQuaternaryInt<name##"g", llvm_v2i64_ty, intr_properties>;
 }
 
 multiclass SystemZQuaternaryIntCCBHF {
   def bs : SystemZQuaternaryIntCC<llvm_v16i8_ty>;
   def hs : SystemZQuaternaryIntCC<llvm_v8i16_ty>;
   def fs : SystemZQuaternaryIntCC<llvm_v4i32_ty>;
 }
 

 //
 // Vector intrinsics
 //
 //===----------------------------------------------------------------------===//
 
 let TargetPrefix = "s390" in {
   def int_s390_lcbb : GCCBuiltin<"__builtin_s390_lcbb">,
                       Intrinsic<[llvm_i32_ty], [llvm_ptr_ty, llvm_i32_ty],
-                                [IntrNoMem]>;
+                                [IntrNoMem, ImmArg<1>]>;
 
   def int_s390_vlbb : GCCBuiltin<"__builtin_s390_vlbb">,
                       Intrinsic<[llvm_v16i8_ty], [llvm_ptr_ty, llvm_i32_ty],
-                                [IntrReadMem, IntrArgMemOnly]>;
+                                [IntrReadMem, IntrArgMemOnly, ImmArg<1>]>;
 
   def int_s390_vll : GCCBuiltin<"__builtin_s390_vll">,
                      Intrinsic<[llvm_v16i8_ty], [llvm_i32_ty, llvm_ptr_ty],
                                [IntrReadMem, IntrArgMemOnly]>;
 
   def int_s390_vpdi : GCCBuiltin<"__builtin_s390_vpdi">,
                       Intrinsic<[llvm_v2i64_ty],
                                 [llvm_v2i64_ty, llvm_v2i64_ty, llvm_i32_ty],
-                                [IntrNoMem]>;
+                                [IntrNoMem, ImmArg<2>]>;
 
   def int_s390_vperm : GCCBuiltin<"__builtin_s390_vperm">,
                        Intrinsic<[llvm_v16i8_ty],
                                  [llvm_v16i8_ty, llvm_v16i8_ty, llvm_v16i8_ty],
                                  [IntrNoMem]>;
 
   defm int_s390_vpks : SystemZBinaryTruncHFG<"vpks">;
   defm int_s390_vpks : SystemZBinaryTruncCCHFG;
let TargetPrefix = "s390" in {
   defm int_s390_vmlh : SystemZBinaryBHF<"vmlh">;
   defm int_s390_vme  : SystemZBinaryExtBHF<"vme">;
   defm int_s390_vmle : SystemZBinaryExtBHF<"vmle">;
   defm int_s390_vmo  : SystemZBinaryExtBHF<"vmo">;
   defm int_s390_vmlo : SystemZBinaryExtBHF<"vmlo">;
 
   defm int_s390_verllv : SystemZBinaryBHFG<"verllv">;
   defm int_s390_verll  : SystemZBinaryIntBHFG<"verll">;
-  defm int_s390_verim  : SystemZQuaternaryIntBHFG<"verim">;
+  defm int_s390_verim  : SystemZQuaternaryIntBHFG<"verim", [ImmArg<3>]>;
 
   def int_s390_vsl   : SystemZBinary<"vsl",   llvm_v16i8_ty>;
   def int_s390_vslb  : SystemZBinary<"vslb",  llvm_v16i8_ty>;
   def int_s390_vsra  : SystemZBinary<"vsra",  llvm_v16i8_ty>;
   def int_s390_vsrab : SystemZBinary<"vsrab", llvm_v16i8_ty>;
   def int_s390_vsrl  : SystemZBinary<"vsrl",  llvm_v16i8_ty>;
   def int_s390_vsrlb : SystemZBinary<"vsrlb", llvm_v16i8_ty>;
 
   def int_s390_vsldb : GCCBuiltin<"__builtin_s390_vsldb">,
                        Intrinsic<[llvm_v16i8_ty],
                                  [llvm_v16i8_ty, llvm_v16i8_ty, llvm_i32_ty],
-                                 [IntrNoMem]>;
+                                 [IntrNoMem, ImmArg<2>]>;
 
   defm int_s390_vscbi : SystemZBinaryBHFG<"vscbi">;
 
   def int_s390_vsq     : SystemZBinary<"vsq",      llvm_v16i8_ty>;
   def int_s390_vsbiq   : SystemZTernary<"vsbiq",   llvm_v16i8_ty>;
   def int_s390_vscbiq  : SystemZBinary<"vscbiq",   llvm_v16i8_ty>;
   def int_s390_vsbcbiq : SystemZTernary<"vsbcbiq", llvm_v16i8_ty>;
 
def int_s390_vsumqg : SystemZBinaryConv<"vsumqg", llvm_v16i8_ty,
                                           llvm_v2i64_ty>;
 
   def int_s390_vtm : SystemZBinaryConv<"vtm", llvm_i32_ty, llvm_v16i8_ty>;
 
   defm int_s390_vceq : SystemZCompareBHFG<"vceq">;
   defm int_s390_vch  : SystemZCompareBHFG<"vch">;
   defm int_s390_vchl : SystemZCompareBHFG<"vchl">;
 
-  defm int_s390_vfae  : SystemZTernaryIntBHF<"vfae">;
+  defm int_s390_vfae  : SystemZTernaryIntBHF<"vfae", [ImmArg<2>]>;
   defm int_s390_vfae  : SystemZTernaryIntCCBHF;
-  defm int_s390_vfaez : SystemZTernaryIntBHF<"vfaez">;
+  defm int_s390_vfaez : SystemZTernaryIntBHF<"vfaez", [ImmArg<2>]>;
   defm int_s390_vfaez : SystemZTernaryIntCCBHF;
 
   defm int_s390_vfee  : SystemZBinaryBHF<"vfee">;
   defm int_s390_vfee  : SystemZBinaryCCBHF;
   defm int_s390_vfeez : SystemZBinaryBHF<"vfeez">;
   defm int_s390_vfeez : SystemZBinaryCCBHF;
 
   defm int_s390_vfene  : SystemZBinaryBHF<"vfene">;
   defm int_s390_vfene  : SystemZBinaryCCBHF;
   defm int_s390_vfenez : SystemZBinaryBHF<"vfenez">;
   defm int_s390_vfenez : SystemZBinaryCCBHF;
 
   defm int_s390_vistr : SystemZUnaryBHF<"vistr">;
   defm int_s390_vistr : SystemZUnaryCCBHF;
 
-  defm int_s390_vstrc  : SystemZQuaternaryIntBHF<"vstrc">;
+  defm int_s390_vstrc  : SystemZQuaternaryIntBHF<"vstrc", [ImmArg<3>]>;
   defm int_s390_vstrc  : SystemZQuaternaryIntCCBHF;
-  defm int_s390_vstrcz : SystemZQuaternaryIntBHF<"vstrcz">;
+  defm int_s390_vstrcz : SystemZQuaternaryIntBHF<"vstrcz", [ImmArg<3>]>;
   defm int_s390_vstrcz : SystemZQuaternaryIntCCBHF;
 
   def int_s390_vfcedbs  : SystemZBinaryConvCC<llvm_v2i64_ty, llvm_v2f64_ty>;
   def int_s390_vfchdbs  : SystemZBinaryConvCC<llvm_v2i64_ty, llvm_v2f64_ty>;
   def int_s390_vfchedbs : SystemZBinaryConvCC<llvm_v2i64_ty, llvm_v2f64_ty>;
 
   def int_s390_vftcidb : SystemZBinaryConvIntCC<llvm_v2i64_ty, llvm_v2f64_ty>;
 
   def int_s390_vfidb : Intrinsic<[llvm_v2f64_ty],
                                  [llvm_v2f64_ty, llvm_i32_ty, llvm_i32_ty],
                                  [IntrNoMem]>;
 
   // Instructions from the Vector Enhancements Facility 1
   def int_s390_vbperm : SystemZBinaryConv<"vbperm", llvm_v2i64_ty,
                                           llvm_v16i8_ty>;
 
   def int_s390_vmslg  : GCCBuiltin<"__builtin_s390_vmslg">,
                         Intrinsic<[llvm_v16i8_ty],
                                   [llvm_v2i64_ty, llvm_v2i64_ty, llvm_v16i8_ty,
-                                   llvm_i32_ty], [IntrNoMem]>;
+                                   llvm_i32_ty], [IntrNoMem, ImmArg<3>]>;
 
   def int_s390_vfmaxdb : Intrinsic<[llvm_v2f64_ty],
                                    [llvm_v2f64_ty, llvm_v2f64_ty, llvm_i32_ty],
                                    [IntrNoMem]>;
   def int_s390_vfmindb : Intrinsic<[llvm_v2f64_ty],
                                    [llvm_v2f64_ty, llvm_v2f64_ty, llvm_i32_ty],
                                    [IntrNoMem]>;
   def int_s390_vfmaxsb : Intrinsic<[llvm_v4f32_ty],
                                    [llvm_v4f32_ty, llvm_v4f32_ty, llvm_i32_ty],
                                    [IntrNoMem]>;
   def int_s390_vfminsb : Intrinsic<[llvm_v4f32_ty],
                                    [llvm_v4f32_ty, llvm_v4f32_ty, llvm_i32_ty],
                                    [IntrNoMem]>;
 
   def int_s390_vfcesbs  : SystemZBinaryConvCC<llvm_v4i32_ty, llvm_v4f32_ty>;
   def int_s390_vfchsbs  : SystemZBinaryConvCC<llvm_v4i32_ty, llvm_v4f32_ty>;
   def int_s390_vfchesbs : SystemZBinaryConvCC<llvm_v4i32_ty, llvm_v4f32_ty>;
 
   def int_s390_vftcisb : SystemZBinaryConvIntCC<llvm_v4i32_ty, llvm_v4f32_ty>;
 
   def int_s390_vfisb : Intrinsic<[llvm_v4f32_ty],
                                  [llvm_v4f32_ty, llvm_i32_ty, llvm_i32_ty],
                                  [IntrNoMem]>;
 
   // Instructions from the Vector Packed Decimal Facility
   def int_s390_vlrl : GCCBuiltin<"__builtin_s390_vlrl">,
                       Intrinsic<[llvm_v16i8_ty], [llvm_i32_ty, llvm_ptr_ty],
                                 [IntrReadMem, IntrArgMemOnly]>;
 
   def int_s390_vstrl : GCCBuiltin<"__builtin_s390_vstrl">,
                        Intrinsic<[], [llvm_v16i8_ty, llvm_i32_ty, llvm_ptr_ty],