[PATCH] D58353: SystemZ: Add ImmArg to intrinsics

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

In D58353#1401332 <https://reviews.llvm.org/D58353#1401332>, @uweigand wrote:

> 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.)


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


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

https://reviews.llvm.org/D58353