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