David Candler <david.cand...@arm.com> writes:
> Hi Richard,
>
> Thanks for the feedback.
>
> Richard Sandiford <richard.sandif...@arm.com> writes:
>> > diff --git a/gcc/config/aarch64/aarch64-builtins.c
>> > b/gcc/config/aarch64/aarch64-builtins.c
>> > index 4f33dd936c7..f93f4e29c89 100644
>> > --- a/gcc/config/aarch64/aarch64-builtins.c
>> > +++ b/gcc/config/aarch64/aarch64-builtins.c
>> > @@ -254,6 +254,10 @@
>> > aarch64_types_binop_imm_qualifiers[SIMD_MAX_BUILTIN_ARGS]
>> > #define TYPES_GETREG (aarch64_types_binop_imm_qualifiers)
>> > #define TYPES_SHIFTIMM (aarch64_types_binop_imm_qualifiers)
>> > static enum aarch64_type_qualifiers
>> > +aarch64_types_ternop_s_imm_qualifiers[SIMD_MAX_BUILTIN_ARGS]
>> > + = { qualifier_none, qualifier_none, qualifier_none,
>> > qualifier_immediate};
>> > +#define TYPES_SHIFT2IMM (aarch64_types_ternop_s_imm_qualifiers)
>> > +static enum aarch64_type_qualifiers
>> > aarch64_types_shift_to_unsigned_qualifiers[SIMD_MAX_BUILTIN_ARGS]
>> > = { qualifier_unsigned, qualifier_none, qualifier_immediate };
>> > #define TYPES_SHIFTIMM_USS (aarch64_types_shift_to_unsigned_qualifiers)
>> > @@ -265,14 +269,16 @@ static enum aarch64_type_qualifiers
>> > aarch64_types_unsigned_shift_qualifiers[SIMD_MAX_BUILTIN_ARGS]
>> > = { qualifier_unsigned, qualifier_unsigned, qualifier_immediate };
>> > #define TYPES_USHIFTIMM (aarch64_types_unsigned_shift_qualifiers)
>> > +#define TYPES_USHIFT2IMM (aarch64_types_ternopu_imm_qualifiers)
>> > +static enum aarch64_type_qualifiers
>> > +aarch64_types_shift2_to_unsigned_qualifiers[SIMD_MAX_BUILTIN_ARGS]
>> > + = { qualifier_unsigned, qualifier_unsigned, qualifier_none,
>> > qualifier_immediate };
>> > +#define TYPES_SHIFT2IMM_UUSS (aarch64_types_shift2_to_unsigned_qualifiers)
>> >
>> > static enum aarch64_type_qualifiers
>> > aarch64_types_ternop_s_imm_p_qualifiers[SIMD_MAX_BUILTIN_ARGS]
>> > = { qualifier_none, qualifier_none, qualifier_poly,
>> > qualifier_immediate};
>> > #define TYPES_SETREGP (aarch64_types_ternop_s_imm_p_qualifiers)
>> > -static enum aarch64_type_qualifiers
>> > -aarch64_types_ternop_s_imm_qualifiers[SIMD_MAX_BUILTIN_ARGS]
>> > - = { qualifier_none, qualifier_none, qualifier_none,
>> > qualifier_immediate};
>> > #define TYPES_SETREG (aarch64_types_ternop_s_imm_qualifiers)
>> > #define TYPES_SHIFTINSERT (aarch64_types_ternop_s_imm_qualifiers)
>> > #define TYPES_SHIFTACC (aarch64_types_ternop_s_imm_qualifiers)
>>
>> Very minor, but I think it would be better to keep
>> aarch64_types_ternop_s_imm_qualifiers where it is and define
>> TYPES_SHIFT2IMM here rather than above. For better or worse,
>> the current style seems to be to keep the defines next to the
>> associated arrays, rather than group them based on the TYPES_* name.
>>
>> > diff --git a/gcc/config/aarch64/aarch64-simd-builtins.def
>> > b/gcc/config/aarch64/aarch64-simd-builtins.def
>> > index d1b21102b2f..0b82b9c072b 100644
>> > --- a/gcc/config/aarch64/aarch64-simd-builtins.def
>> > +++ b/gcc/config/aarch64/aarch64-simd-builtins.def
>> > @@ -285,6 +285,13 @@
>> > BUILTIN_VSQN_HSDI (USHIFTIMM, uqshrn_n, 0, ALL)
>> > BUILTIN_VSQN_HSDI (SHIFTIMM, sqrshrn_n, 0, ALL)
>> > BUILTIN_VSQN_HSDI (USHIFTIMM, uqrshrn_n, 0, ALL)
>> > + /* Implemented by aarch64_<sur>q<r>shr<u>n2_n<mode>. */
>> > + BUILTIN_VQN (SHIFT2IMM_UUSS, sqshrun2_n, 0, ALL)
>> > + BUILTIN_VQN (SHIFT2IMM_UUSS, sqrshrun2_n, 0, ALL)
>> > + BUILTIN_VQN (SHIFT2IMM, sqshrn2_n, 0, ALL)
>> > + BUILTIN_VQN (USHIFT2IMM, uqshrn2_n, 0, ALL)
>> > + BUILTIN_VQN (SHIFT2IMM, sqrshrn2_n, 0, ALL)
>> > + BUILTIN_VQN (USHIFT2IMM, uqrshrn2_n, 0, ALL)
>>
>> Using ALL is a holdover from the time (until a few weeks ago) when we
>> didn't record function attributes. New intrinsics should therefore
>> have something more specific than ALL.
>>
>> We discussed offline whether the Q flag side effect of the intrinsics
>> should be observable or not, and the conclusion was that it shouldn't.
>> I think we can therefore treat these functions as pure functions,
>> meaning that they should have flags NONE rather than ALL.
>>
>> For that reason, I think we should also remove the Set_Neon_Cumulative_Sat
>> and CHECK_CUMULATIVE_SAT parts of the test (sorry).
>>
>> Other than that, the patch looks good to go.
>>
>> Thanks,
>> Richard
>
> I've updated the patch with TYPES_SHIFT2IMM moved, the builtins changed
> to NONE, and the Q flag portion of the tests removed.
Looks good to me, thanks. Pushed to trunk.
Richard
