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.
Details
- Reviewers
uweigand
Diff Detail
Event Timeline
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.)
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 | ||
---|---|---|
47 | There should be no need to change this. | |
53 | Or this. | |
56 | Just hard-code ImmArg<2> here, it will be needed by all users. | |
65 | Likewise just hard-code ImmArg<3>. | |
170 | This change can then go away. | |
182 | And this. | |
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>. |
There should be no need to change this.