Re: [PATCH] testsuite: update requires for powerpc/float128-cmp2-runnable.c

["Kewen.Lin" <linkw@linux.ibm.com>]

Hi Jeff,

on 2023/4/13 15:45, guojiufu wrote:
> Hi,
> 
> On 2023-04-12 20:47, Kewen.Lin wrote:
>> Hi Segher & Jeff,
>>
>> on 2023/4/11 23:13, Segher Boessenkool wrote:
>>> On Tue, Apr 11, 2023 at 05:40:09PM +0800, Kewen.Lin wrote:
>>>> on 2023/4/11 17:14, guojiufu wrote:
>>>>> Thanks for raising this concern.
>>>>> The behavior to check about bif on FLOAT128_HW and emit an error message for
>>>>> requirements on quad-precision is added in gcc12. This is why gcc12 fails to
>>>>> compile the case on -m32.
>>>>>
>>>>> Before gcc12, altivec_resolve_overloaded_builtin will return the overloaded
>>>>> result directly, and does not check more about the result function.
>>>>
>>>> Thanks for checking, I wonder which commit caused this behavior change and what's
>>>> the underlying justification?  I know there is one new bif handling framework
>>
>> Answered this question by myself with some diggings, test case
>> float128-cmp2-runnable.c started to fail from r12-5752-gd08236359eb229 which
>> exactly makes new bif framework start to take effect and the reason why the
>> behavior changes is the condition change from **TARGET_P9_VECTOR** to
>> **TARGET_FLOAT128_HW**.
>>
>> With r12-5751-gc9dd01314d8467 (still old bif framework):
>>
>> $ grep -r scalar_cmp_exp_qp gcc/config/rs6000/rs6000-builtin.def
>> BU_P9V_VSX_2 (VSCEQPGT, "scalar_cmp_exp_qp_gt", CONST,  xscmpexpqp_gt_kf)
>> BU_P9V_VSX_2 (VSCEQPLT, "scalar_cmp_exp_qp_lt", CONST,  xscmpexpqp_lt_kf)
>> BU_P9V_VSX_2 (VSCEQPEQ, "scalar_cmp_exp_qp_eq", CONST,  xscmpexpqp_eq_kf)
>> BU_P9V_VSX_2 (VSCEQPUO, "scalar_cmp_exp_qp_unordered",  CONST,
>> xscmpexpqp_unordered_kf)
>> BU_P9V_OVERLOAD_2 (VSCEQPGT,    "scalar_cmp_exp_qp_gt")
>> BU_P9V_OVERLOAD_2 (VSCEQPLT,    "scalar_cmp_exp_qp_lt")
>> BU_P9V_OVERLOAD_2 (VSCEQPEQ,    "scalar_cmp_exp_qp_eq")
>> BU_P9V_OVERLOAD_2 (VSCEQPUO,    "scalar_cmp_exp_qp_unordered")
>>
>> There were only 13 bifs requiring TARGET_FLOAT128_HW in old bif framework.
>>
>> $ grep ^BU_FLOAT128_HW gcc/config/rs6000/rs6000-builtin.def
>> BU_FLOAT128_HW_VSX_1 (VSEEQP,   "scalar_extract_expq",  CONST,  xsxexpqp_kf)
>> BU_FLOAT128_HW_VSX_1 (VSESQP,   "scalar_extract_sigq",  CONST,  xsxsigqp_kf)
>> BU_FLOAT128_HW_VSX_1 (VSTDCNQP, "scalar_test_neg_qp",   CONST,  xststdcnegqp_kf)
>> BU_FLOAT128_HW_VSX_2 (VSIEQP,   "scalar_insert_exp_q",  CONST,  xsiexpqp_kf)
>> BU_FLOAT128_HW_VSX_2 (VSIEQPF,  "scalar_insert_exp_qp", CONST,  xsiexpqpf_kf)
>> BU_FLOAT128_HW_VSX_2 (VSTDCQP, "scalar_test_data_class_qp",     CONST,
>>  xststdcqp_kf)
>> BU_FLOAT128_HW_1 (SQRTF128_ODD,  "sqrtf128_round_to_odd",  FP, sqrtkf2_odd)
>> BU_FLOAT128_HW_1 (TRUNCF128_ODD, "truncf128_round_to_odd", FP, trunckfdf2_odd)
>> BU_FLOAT128_HW_2 (ADDF128_ODD,   "addf128_round_to_odd",   FP, addkf3_odd)
>> BU_FLOAT128_HW_2 (SUBF128_ODD,   "subf128_round_to_odd",   FP, subkf3_odd)
>> BU_FLOAT128_HW_2 (MULF128_ODD,   "mulf128_round_to_odd",   FP, mulkf3_odd)
>> BU_FLOAT128_HW_2 (DIVF128_ODD,   "divf128_round_to_odd",   FP, divkf3_odd)
>> BU_FLOAT128_HW_3 (FMAF128_ODD,   "fmaf128_round_to_odd",   FP, fmakf4_odd)
>>
>> Starting from r12-5752-gd08236359eb229, these
>> scalar_cmp_exp_qp_{gt,lt,eq,unordered}
>> bifs were put under stanza ieee128-hw, it makes ieee128-hw to have 17 bifs,
>> comparing to the previous, the extra four ones were exactly these
>> scalar_cmp_exp_qp_{gt,lt,eq,unordered}.
>>
>>>> introduced in gcc12, not sure the checking condition was changed together or by
>>>> a standalone commit.  Anyway, apparently the conditions for the support of these
>>>> bifs are different on gcc-11 and gcc-12, I wonder why it changed.  As mentioned
>>>> above, PR108758's c#1 said this case (bifs) work well on gcc-11, I suspected the
>>>> condition change was an overkill, that's why I asked.
>>>
>>> It almost certainly was an oversight.  The new builtin framework changed
>>> so many things, there was bound to be some breakage to go with all the
>>> good things it brought.
>>
>> Yeah, as the above findings, also I found that
>> r12-3126-g2ed356a4c9af06 introduced
>> power9 related stanzas and r12-3167-g2f9489a1009d98 introduced ieee128-hw stanza
>> including these four bifs, both of them don't have any notes on why we
>> would change
>> the condition for these scalar_cmp_exp_qp_{gt,lt,eq,unordered} from
>> power9-vector to
>> ieee128-hw, so I think it's just an oversight (ieee128-hw is an
>> overkill comparing
>> to power9-vector :)).
>>
>>>
>>> So what is the actual thing going wrong?  QP insns work fine and are
>>> valid on all systems and environments, BE or LE, 32-bit or 64-bit.  Of
>>> course you cannot use the "long double" type for those everywhere, but
>>> that is a very different thing.
>>
>> The actual thing going wrong is that: the test case float128-cmp2-runnable.c
>> runs well on BE -m32 and -m64 with gcc-11, but meets failures on BE -m32 with
>> latest gcc-12 and trunk during compilation, having the error messages like:
>>
>> gcc/testsuite/gcc.target/powerpc/float128-cmp2-runnable.c: In function 'main':
>> gcc/testsuite/gcc.target/powerpc/float128-cmp2-runnable.c:155:3: error:
>>   '__builtin_vsx_scalar_cmp_exp_qp_eq' requires ISA 3.0 IEEE 128-bit
>> floating point
>>
>> As scalar_cmp_exp_qp_{gt,lt,eq,unordered} requires condition TARGET_FLOAT128_HW
>> now (since new bif framework took effect).
>>
>> (To be more exact, it started to fail from r12-5752-gd08236359eb229).
>>
>> IMHO, the apparent cause seems to be the wrong effective target mismatching the
>> condition for those bifs, but the underlying cause is that new bif framework
>> unexpectedly moved these four bifs from power9-vector to ieee128-hw.
>>
>> Testing a diff as below:
>>
>> diff --git a/gcc/config/rs6000/rs6000-builtins.def
>> b/gcc/config/rs6000/rs6000-builtins.def
>> index 03fb194b151..67a3f5edaf2 100644
>> --- a/gcc/config/rs6000/rs6000-builtins.def
>> +++ b/gcc/config/rs6000/rs6000-builtins.def
>> @@ -2797,6 +2797,19 @@
>>    const vsi __builtin_vsx_xxbrw_v4si (vsi);
>>      XXBRW_V4SI p9_xxbrw_v4si {}
>>
>> +  const signed int __builtin_vsx_scalar_cmp_exp_qp_eq (_Float128, _Float128);
>> +    VSCEQPEQ xscmpexpqp_eq_kf {}
>> +
>> +  const signed int __builtin_vsx_scalar_cmp_exp_qp_gt (_Float128, _Float128);
>> +    VSCEQPGT xscmpexpqp_gt_kf {}
>> +
>> +  const signed int __builtin_vsx_scalar_cmp_exp_qp_lt (_Float128, _Float128);
>> +    VSCEQPLT xscmpexpqp_lt_kf {}
>> +
>> +  const signed int \
>> +      __builtin_vsx_scalar_cmp_exp_qp_unordered (_Float128, _Float128);
>> +    VSCEQPUO xscmpexpqp_unordered_kf {}
>> +
>>
>>  ; Miscellaneous P9 functions
>>  [power9]
>> @@ -2879,19 +2892,6 @@
>>    fpmath _Float128 __builtin_mulf128_round_to_odd (_Float128, _Float128);
>>      MULF128_ODD mulkf3_odd {}
>>
>> -  const signed int __builtin_vsx_scalar_cmp_exp_qp_eq (_Float128, _Float128);
>> -    VSCEQPEQ xscmpexpqp_eq_kf {}
>> -
>> -  const signed int __builtin_vsx_scalar_cmp_exp_qp_gt (_Float128, _Float128);
>> -    VSCEQPGT xscmpexpqp_gt_kf {}
>> -
>> -  const signed int __builtin_vsx_scalar_cmp_exp_qp_lt (_Float128, _Float128);
>> -    VSCEQPLT xscmpexpqp_lt_kf {}
>> -
>> -  const signed int \
>> -      __builtin_vsx_scalar_cmp_exp_qp_unordered (_Float128, _Float128);
>> -    VSCEQPUO xscmpexpqp_unordered_kf {}
>> -
>>    fpmath _Float128 __builtin_sqrtf128_round_to_odd (_Float128);
>>      SQRTF128_ODD sqrtkf2_odd {}
>>
> 
> Thanks a lot for your great findings and comments!
> 
> I understand your points:
> 1.  This is `regression`:  float128-cmp2-runnable.c pass and runs well on gcc-11.
>     But fail to compile with gcc12 and trunk.
> 2.  Trunk(or gcc-12) fail to compile this case because the bifs
>     (e.g. __builtin_vsx_scalar_cmp_exp_qp_eq) are put under [ieee128-hw].
>     (This may be an unexpected/unintended change.)
> and 3. xscmpexpqp (QP insn) is valid on power9 system (no matter BE/LE, -m32/-64):
>     define_insn for xscmpexpqp's condition is "TARGET_P9_VECTOR".
> 
> So, We may fix the regression first.
>     "updating those bifs (scalar_cmp_exp_qp) to make them can be compiled on P9/BE/-m32"
>     would be a fix for this regression.
> 

Thanks for the summary, yeah, this is a regression IMHO.  Those bifs worked well on
Power9 BE -m32 in the past, it's not sensible to disable them there.  Also considering
the corresponding define_insn condition and commit history etc, I believe it's an
oversight, and we should fix the sitting stanza instead of the test case.

BR,
Kewen