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

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

[Bill Schmidt]

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?

Thanks!
Bill


2021-12-13  Bill Schmidt  <wschmidt@linux.ibm.com>

gcc/
	PR target/103624
	* config/rs6000/rs6000-builtin-new.def (__builtin_darn): Move to
	[power9-64] stanza.
	(__builtin_darn_raw): Likewise.
---
 gcc/config/rs6000/rs6000-builtin-new.def | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/gcc/config/rs6000/rs6000-builtin-new.def b/gcc/config/rs6000/rs6000-builtin-new.def
index 30556e5c7f2..2becd96a36c 100644
--- a/gcc/config/rs6000/rs6000-builtin-new.def
+++ b/gcc/config/rs6000/rs6000-builtin-new.def
@@ -2799,15 +2799,9 @@
 
 ; Miscellaneous P9 functions
 [power9]
-  signed long long __builtin_darn ();
-    DARN darn {}
-
   signed int __builtin_darn_32 ();
     DARN_32 darn_32 {}
 
-  signed long long __builtin_darn_raw ();
-    DARN_RAW darn_raw {}
-
   const signed int __builtin_dtstsfi_eq_dd (const int<6>, _Decimal64);
     TSTSFI_EQ_DD dfptstsfi_eq_dd {}
 
@@ -2840,6 +2834,12 @@
   void __builtin_altivec_stxvl (vsc, void *, long);
     STXVL stxvl {}
 
+  signed long long __builtin_darn ();
+    DARN darn {}
+
+  signed long long __builtin_darn_raw ();
+    DARN_RAW darn_raw {}
+
   const signed int __builtin_scalar_byte_in_set (signed int, signed long long);
     CMPEQB cmpeqb {}
 
-- 
2.27.0

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

[David Edelsohn]

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.

Thanks, David

>
> Thanks!
> Bill
>
>
> 2021-12-13  Bill Schmidt  <wschmidt@linux.ibm.com>
>
> gcc/
>         PR target/103624
>         * config/rs6000/rs6000-builtin-new.def (__builtin_darn): Move to
>         [power9-64] stanza.
>         (__builtin_darn_raw): Likewise.
> ---
>  gcc/config/rs6000/rs6000-builtin-new.def | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/gcc/config/rs6000/rs6000-builtin-new.def b/gcc/config/rs6000/rs6000-builtin-new.def
> index 30556e5c7f2..2becd96a36c 100644
> --- a/gcc/config/rs6000/rs6000-builtin-new.def
> +++ b/gcc/config/rs6000/rs6000-builtin-new.def
> @@ -2799,15 +2799,9 @@
>
>  ; Miscellaneous P9 functions
>  [power9]
> -  signed long long __builtin_darn ();
> -    DARN darn {}
> -
>    signed int __builtin_darn_32 ();
>      DARN_32 darn_32 {}
>
> -  signed long long __builtin_darn_raw ();
> -    DARN_RAW darn_raw {}
> -
>    const signed int __builtin_dtstsfi_eq_dd (const int<6>, _Decimal64);
>      TSTSFI_EQ_DD dfptstsfi_eq_dd {}
>
> @@ -2840,6 +2834,12 @@
>    void __builtin_altivec_stxvl (vsc, void *, long);
>      STXVL stxvl {}
>
> +  signed long long __builtin_darn ();
> +    DARN darn {}
> +
> +  signed long long __builtin_darn_raw ();
> +    DARN_RAW darn_raw {}
> +
>    const signed int __builtin_scalar_byte_in_set (signed int, signed long long);
>      CMPEQB cmpeqb {}
>
> --
> 2.27.0
>
>

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

[Segher Boessenkool]

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!


Segher


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

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

[Bill Schmidt]

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.

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

[Segher Boessenkool]

On Mon, Dec 13, 2021 at 02:37:43PM -0600, Bill Schmidt wrote:
> 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:
> >>> 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?

That of course does not work for _raw.

These builtins should just return a "long", just like __builtin_ppc_mftb
does.  All three of them.

> 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.

Avoiding ICEs should not be a goal.  It should be a side effect of doing
the right thing in the first place!


Segher

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

[Bill Schmidt]

Hi!

On 12/13/21 6:22 PM, Segher Boessenkool wrote:
> On Mon, Dec 13, 2021 at 02:37:43PM -0600, Bill Schmidt wrote:
>> 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:
>>>>> 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?
> That of course does not work for _raw.
>
> These builtins should just return a "long", just like __builtin_ppc_mftb
> does.  All three of them.

Well, that seems wrong for __builtin_darn_32, which maps to an SImode pattern.

So, I assume what you'd like to see is for the other two built-ins to return
long, and for the "&& TARGET_64BIT" to be removed from the darn_raw and darn
patterns?

>
>> 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.
> Avoiding ICEs should not be a goal.  It should be a side effect of doing
> the right thing in the first place!


There's no reason to get snippy.  Given that you approved Kelvin's original
implementation of the darn patterns and built-in functions, I think I can be
forgiven for thinking that those were the desired semantics. :-)

Thanks,
Bill

>
>
> Segher

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

[Bill Schmidt]

On 12/14/21 7:32 AM, Bill Schmidt wrote:
> Hi!
>
> On 12/13/21 6:22 PM, Segher Boessenkool wrote:
>>
>> These builtins should just return a "long", just like __builtin_ppc_mftb
>> does.  All three of them.
> Well, that seems wrong for __builtin_darn_32, which maps to an SImode pattern.
>
> So, I assume what you'd like to see is for the other two built-ins to return
> long, and for the "&& TARGET_64BIT" to be removed from the darn_raw and darn
> patterns?
>
For the record, I don't see how this can work.  WHen I compile:

#include <altivec.h>

long get_raw_random ()
{
  return __builtin_darn_raw ();
}

with these changes, the compiler thinks that __builtin_darn_raw returns a
register pair, presumably due to it being a DImode pattern.  It then pulls
the second register of the pair as the actual result.

get_raw_random:
.LFB0:
        darn 10,2
        mr 3,11
        blr

The vregs dump shows:

(insn 5 2 6 2 (set (reg:DI 118)
        (unspec_volatile:DI [
                (const_int 0 [0])
            ] UNSPECV_DARN_RAW)) "darn-thing.c":11:10 1043 {darn_raw}
     (nil))
(insn 6 5 10 2 (set (reg:SI 117 [ <retval> ])
        (subreg:SI (reg:DI 118) 4)) "darn-thing.c":11:10 543 {*movsi_internal1}
     (nil))
(insn 10 6 11 2 (set (reg/i:SI 3 3)
        (reg:SI 117 [ <retval> ])) "darn-thing.c":12:1 543 {*movsi_internal1}
     (nil))
(insn 11 10 0 2 (use (reg/i:SI 3 3)) "darn-thing.c":12:1 -1
     (nil))

So if you want to support these patterns for 32-bit mode, there's more work
required.

Given this, I'd like to ask you to reconsider the original submitted patch
for now.

Thanks,
Bill

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

[Segher Boessenkool]

On Tue, Dec 14, 2021 at 07:32:30AM -0600, Bill Schmidt wrote:
> On 12/13/21 6:22 PM, Segher Boessenkool wrote:
> > On Mon, Dec 13, 2021 at 02:37:43PM -0600, Bill Schmidt wrote:
> >> 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:
> >>>>> 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?
> > That of course does not work for _raw.
> >
> > These builtins should just return a "long", just like __builtin_ppc_mftb
> > does.  All three of them.
> 
> Well, that seems wrong for __builtin_darn_32, which maps to an SImode pattern.

That is Yet Another Bug, then.

The insn returns a full register.  The patterns should use either :P or
:GPR (the latter if SImode makes sense for it, so we could have that for
all darn variants).  :DI and :SI never make sense for this.

> So, I assume what you'd like to see is for the other two built-ins to return
> long, and for the "&& TARGET_64BIT" to be removed from the darn_raw and darn
> patterns?

No, all builtins should work in either mode, and always return long.
If the patterns are broken, the *patterns* should be fixed :-)

> > Avoiding ICEs should not be a goal.  It should be a side effect of doing
> > the right thing in the first place!
> 
> There's no reason to get snippy.  Given that you approved Kelvin's original
> implementation of the darn patterns and built-in functions, I think I can be
> forgiven for thinking that those were the desired semantics. :-)

Sorry if I sound annoyed.  I am annoyed, but not with you.  Just with
the world in general I suppose.

With the new builtins representation it is much easier to spot problems,
it is a great success already!


Segher

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

[Bill Schmidt]

On 12/14/21 8:23 PM, Segher Boessenkool wrote:
> On Tue, Dec 14, 2021 at 07:32:30AM -0600, Bill Schmidt wrote:
>> On 12/13/21 6:22 PM, Segher Boessenkool wrote:
>>> On Mon, Dec 13, 2021 at 02:37:43PM -0600, Bill Schmidt wrote:
>>>> 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:
>>>>>>> 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?
>>> That of course does not work for _raw.
>>>
>>> These builtins should just return a "long", just like __builtin_ppc_mftb
>>> does.  All three of them.
>> Well, that seems wrong for __builtin_darn_32, which maps to an SImode pattern.
> That is Yet Another Bug, then.
>
> The insn returns a full register.  The patterns should use either :P or
> :GPR (the latter if SImode makes sense for it, so we could have that for
> all darn variants).  :DI and :SI never make sense for this.
>
>> So, I assume what you'd like to see is for the other two built-ins to return
>> long, and for the "&& TARGET_64BIT" to be removed from the darn_raw and darn
>> patterns?
> No, all builtins should work in either mode, and always return long.
> If the patterns are broken, the *patterns* should be fixed :-)


OK, thanks!  This is much clearer now.

I've opened an internal issue about the deficiencies of the darn patterns and
their associated built-ins.  In response to PR103624, I would like to start
with the existing patch to ensure the new support mirrors what we had before,
so we have that as a baseline.  We can then move on to fixing the larger
set of problems.  Is that a reasonable plan?

Thanks!
Bill

>
>>> Avoiding ICEs should not be a goal.  It should be a side effect of doing
>>> the right thing in the first place!
>> There's no reason to get snippy.  Given that you approved Kelvin's original
>> implementation of the darn patterns and built-in functions, I think I can be
>> forgiven for thinking that those were the desired semantics. :-)
> Sorry if I sound annoyed.  I am annoyed, but not with you.  Just with
> the world in general I suppose.
>
> With the new builtins representation it is much easier to spot problems,
> it is a great success already!
>
>
> Segher

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

[Segher Boessenkool]

On Wed, Dec 15, 2021 at 08:00:02AM -0600, Bill Schmidt wrote:
> > No, all builtins should work in either mode, and always return long.
> > If the patterns are broken, the *patterns* should be fixed :-)
> 
> OK, thanks!  This is much clearer now.
> 
> I've opened an internal issue about the deficiencies of the darn patterns and
> their associated built-ins.  In response to PR103624, I would like to start
> with the existing patch to ensure the new support mirrors what we had before,
> so we have that as a baseline.  We can then move on to fixing the larger
> set of problems.  Is that a reasonable plan?

It is much more work than doing it correct in the first place.

I'll do the RTL side, if you want?


Segher

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

[Bill Schmidt]

On 12/15/21 12:41 PM, Segher Boessenkool wrote:
> On Wed, Dec 15, 2021 at 08:00:02AM -0600, Bill Schmidt wrote:
>>> No, all builtins should work in either mode, and always return long.
>>> If the patterns are broken, the *patterns* should be fixed :-)
>> OK, thanks!  This is much clearer now.
>>
>> I've opened an internal issue about the deficiencies of the darn patterns and
>> their associated built-ins.  In response to PR103624, I would like to start
>> with the existing patch to ensure the new support mirrors what we had before,
>> so we have that as a baseline.  We can then move on to fixing the larger
>> set of problems.  Is that a reasonable plan?
> It is much more work than doing it correct in the first place.
>
> I'll do the RTL side, if you want?

Sure, go ahead.

Bill

>
>
> Segher