Re: [PATCH] rs6000: __builtin_darn[_raw] should be in [power9-64] (PR103624)

[Bill Schmidt <wschmidt@linux.ibm.com>]

Hi!

On 12/13/21 10:54 AM, Segher Boessenkool wrote:
> On Mon, Dec 13, 2021 at 11:30:28AM -0500, David Edelsohn wrote:
>> On Mon, Dec 13, 2021 at 10:48 AM Bill Schmidt <wschmidt@linux.ibm.com> wrote:
>>> Hi!
>>>
>>> PR103624 observes that we get segfaults for the 64-bit darn builtins when compiled
>>> on a 32-bit architecture.  The old built-in infrastructure requires TARGET_64BIT, and
>>> this was missed in the new support.  Moving these two builtins from the [power9]
>>> stanza to the [power9-64] stanza solves the problem.
>>>
>>> Tested the fix on a powerpc-e300c3-linux-gnu cross.  Bootstrapped and tested on
>>> powerpc64le-linux-gnu with no regressions.  Is this okay for trunk?
>> Okay.
> No, as I said before this is not correct, not without a lot more
> explanation at least.  We should not copy errors in the old code into
> the new code.  That is negating one of the main advantages of
> reimplementing this in the first place!

Can you please be more specific?

All I have from you before is "It should work for 32-bit though?"  I responded in the
bug report that __builtin_darn_32 was used for this purpose.  I haven't seen a
response to that.  What do you want to see happen?

The patterns in rs6000.md are darn_32, gated by TARGET_P9_MISC; darn_raw, gated by
TARGET_P9_MISC && TARGET_64BIT; and darn, gated by TARGET_P9_MISC && TARGET_64BIT.
The builtins correspond to these patterns in the obvious way.

If you think that these patterns should be enabled differently, that's fine, but
that's a completely different patch than fixing the incorrect built-ins to match
what the patterns do and thus avoid ICEing.

Thanks,
Bill

>
> Segher
>
>
>>>         PR target/103624
>>>         * config/rs6000/rs6000-builtin-new.def (__builtin_darn): Move to
>>>         [power9-64] stanza.
>>>         (__builtin_darn_raw): Likewise.