[PATCH] D58353: SystemZ: Add ImmArg to intrinsics

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

In D58353#1401325 <https://reviews.llvm.org/D58353#1401325>, @arsenm wrote:

> In D58353#1401312 <https://reviews.llvm.org/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.)


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

https://reviews.llvm.org/D58353