Hi Segher & Jakub,
Thanks for the comments!
on 2022/4/7 9:00 PM, Jakub Jelinek wrote:
> On Thu, Apr 07, 2022 at 06:09:52AM -0500, Segher Boessenkool wrote:
>> Hi!
>>
>> On Thu, Mar 03, 2022 at 10:11:32AM +0800, Kewen.Lin wrote:
>>> As PR103623 shows, it's a regression failure due to new built-in
>>> function framework, previously we guard __builtin_{un,}pack_{longdouble,
>>> ibm128} built-in functions under hard float, so they are unavailable
>>> with the given configuration. While with new bif infrastructure, it
>>> becomes available and gets ICE due to incomplete supports.
>>>
>>> Segher and Peter pointed out that we should make them available with
>>> soft float, I agree we can extend them to cover both soft and hard
>>> float. But considering it's stage 4 now and this regression is
>>> classified as P1, also the previous behavior requiring hard float
>>> aligns with what document [1] says, I guess it may be a good idea to
>>> fix it with the attached small patch to be consistent with the previous
>>> behavior. Then we can extend the functionality in upcoming stage 1.
>>
>> Or you could just not take away the existing functionality.
>
> Have those builtins ever worked with 64-bit soft-float?
> When I try e.g. gcc 11 on the #c0 testcase from the PR, I get:
> ./cc1.r11-1 -quiet -nostdinc pr103623.c -m64 -mlong-double-128 -msoft-float
> pr103623.c: In function ‘main’:
> pr103623.c:2:16: error: ‘__builtin_unpack_longdouble’ requires the
> ‘-mhard-float’ option
> 2 | #define UNPACK __builtin_unpack_longdouble
> | ^
> pr103623.c:11:15: note: in expansion of macro ‘UNPACK’
> 11 | double x0 = UNPACK (a, 0);
> | ^~~~~~
>
> From what I can see, those builtins were using:
> /* 128-bit long double floating point builtins. */
> #define BU_LDBL128_2(ENUM, NAME, ATTR, ICODE) \
> RS6000_BUILTIN_2 (MISC_BUILTIN_ ## ENUM, /* ENUM */ \
> "__builtin_" NAME, /* NAME */ \
> (RS6000_BTM_HARD_FLOAT /* MASK */ \
> | RS6000_BTM_LDBL128), \
> (RS6000_BTC_ ## ATTR /* ATTR */ \
> | RS6000_BTC_BINARY), \
> CODE_FOR_ ## ICODE) /* ICODE */
>
> /* 128-bit __ibm128 floating point builtins (use -mfloat128 to indicate that
> __ibm128 is available). */
> #define BU_IBM128_2(ENUM, NAME, ATTR, ICODE) \
> RS6000_BUILTIN_2 (MISC_BUILTIN_ ## ENUM, /* ENUM */ \
> "__builtin_" NAME, /* NAME */ \
> (RS6000_BTM_HARD_FLOAT /* MASK */ \
> | RS6000_BTM_FLOAT128), \
> (RS6000_BTC_ ## ATTR /* ATTR */ \
> | RS6000_BTC_BINARY), \
> CODE_FOR_ ## ICODE) /* ICODE */
> macros and rs6000_builtin_is_supported_p was checking whether
> all the bits in the mask are set:
> HOST_WIDE_INT fnmask = rs6000_builtin_info[fncode].mask;
> if ((fnmask & rs6000_builtin_mask) != fnmask)
> return false;
> else
> return true;
> (so logical and of all of them).
>
As Jakub noted here, we don't have the soft-float support for both m32 and m64
before, as the bifs are always guarded under hard-float previously. I just did
a further check with a powerpc-e300c3-linux-gnu cross-build, the ICE exists for
both m32 and m64.
I guessed the discussions in the PR saying a little more difficulty on m32 than
m64 for extending with soft-float support caused the confusion?
>> What makes it ICE on (at least some configurations of) 32-bit now? Can
>> you exclude just 32-bit soft float?
As clarified above, both 32-bit and 64-bit has the same root cause for the ICE,
the existing define_insn* supports for these bifs only consider hard-float, such
as for the given test case in the PR, it fails in reload as the recognized
unpacktf_nodm doesn't have any available alternatives at soft-float. eg: we
only
have register constraint "d" for
(match_operand:FMOVE128 1 "register_operand" "d,d")
but it's only available for hard-float.
BR,
Kewen
