, rs6000, PR/target 94622, Be more careful with plq for atomic_load<mode>

For future architecture with prefix instructions, always use plq
rather than lq for atomi load of quadword. Then we never have to
do the doubleword swap on little endian. Before this fix, -mno-pcrel
would generate lq with the doubleword swap (which was ok) and -mpcrel
would generate plq, also with the doubleword swap, which was wrong.

OK for trunk if regstrap passes on ppc64le power9?

Thanks,
   Aaron

2020-04-20  Aaron Sawdey  <acsawdey@linux.ibm.com>

	PR target/94622
	* config/rs6000/sync.md (load_quadpti): Make this have attr prefixed
	if TARGET_PREFIXED.
	(atomic_load<mode>): Do not swap doublewords if TARGET_PREFIXED as
	plq will be used and doesn't need it.
---
 gcc/config/rs6000/sync.md | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Hi,

On Mon, 2020-04-20 at 14:00 -0500, Aaron Sawdey via Gcc-patches wrote:
> For future architecture with prefix instructions, always use plq
> rather than lq for atomi load of quadword. Then we never have to

atomic :-)

> do the doubleword swap on little endian. Before this fix, -mno-pcrel
> would generate lq with the doubleword swap (which was ok) and -mpcrel
> would generate plq, also with the doubleword swap, which was wrong.
> 
> OK for trunk if regstrap passes on ppc64le power9?

> 
> Thanks,
>    Aaron
> 
> 2020-04-20  Aaron Sawdey  <acsawdey@linux.ibm.com>
> 
> 	PR target/94622
> 	* config/rs6000/sync.md (load_quadpti): Make this have attr
> prefixed
> 	if TARGET_PREFIXED.

I'd rearrange to be
: Add attr "prefixed" if TARGET_PREFIXED.

but thats mostly cosmetic, so no big deal.  

> 	(atomic_load<mode>): Do not swap doublewords if TARGET_PREFIXED
> as
> 	plq will be used and doesn't need it.

Ok.   


> ---
>  gcc/config/rs6000/sync.md | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/gcc/config/rs6000/sync.md b/gcc/config/rs6000/sync.md
> index f27edc77b6a..64dfda6ef75 100644
> --- a/gcc/config/rs6000/sync.md
> +++ b/gcc/config/rs6000/sync.md
> @@ -129,7 +129,10 @@ (define_insn "load_quadpti"
>    "TARGET_SYNC_TI
>     && !reg_mentioned_p (operands[0], operands[1])"
>    "lq %0,%1"
> -  [(set_attr "type" "load")])
> +  [(set_attr "type" "load")
> +   (set_attr "prefixed" (if_then_else (match_test "TARGET_PREFIXED")
> +                                      (const_string "yes")
> +                                      (const_string "no")))])
> 
>  (define_expand "atomic_load<mode>"
>    [(set (match_operand:AINT 0 "register_operand")		;;
> output
> @@ -162,7 +165,7 @@ (define_expand "atomic_load<mode>"
> 
>        emit_insn (gen_load_quadpti (pti_reg, op1));
> 
> -      if (WORDS_BIG_ENDIAN)
> +      if (WORDS_BIG_ENDIAN || TARGET_PREFIXED)
>  	emit_move_insn (op0, gen_lowpart (TImode, pti_reg));
>        else
>  	{

seems straightforward.  lgtm.  

Thanks
-Will

For future architecture with prefix instructions, always use plq
rather than lq for atomic load of quadword. Then we never have to
do the doubleword swap on little endian. Before this fix, -mno-pcrel
would generate lq with the doubleword swap (which was ok) and -mpcrel
would generate plq, also with the doubleword swap, which was wrong.

So, of course you can't use set_attr with an if_then_else. The below
code actually builds and passes regstrap on ppc64le power9.

OK for trunk?

Thanks,
    Aaron


2020-04-20  Aaron Sawdey  <acsawdey@linux.ibm.com>

	PR target/94622
	* config/rs6000/sync.md (load_quadpti): Add attr "prefixed"
	if TARGET_PREFIXED.
	(atomic_load<mode>): Do not swap doublewords if TARGET_PREFIXED as
	plq will be used and doesn't need it.
---
 gcc/config/rs6000/sync.md | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/gcc/config/rs6000/sync.md b/gcc/config/rs6000/sync.md
index f27edc77b6a..96cef082dd5 100644
--- a/gcc/config/rs6000/sync.md
+++ b/gcc/config/rs6000/sync.md
@@ -129,7 +129,10 @@ (define_insn "load_quadpti"
   "TARGET_SYNC_TI
    && !reg_mentioned_p (operands[0], operands[1])"
   "lq %0,%1"
-  [(set_attr "type" "load")])
+  [(set_attr "type" "load")
+   (set (attr "prefixed") (if_then_else (match_test "TARGET_PREFIXED")
+                                        (const_string "yes")
+                                        (const_string "no")))])
 
 (define_expand "atomic_load<mode>"
   [(set (match_operand:AINT 0 "register_operand")		;; output
@@ -162,7 +165,7 @@ (define_expand "atomic_load<mode>"
 
       emit_insn (gen_load_quadpti (pti_reg, op1));
 
-      if (WORDS_BIG_ENDIAN)
+      if (WORDS_BIG_ENDIAN || TARGET_PREFIXED)
 	emit_move_insn (op0, gen_lowpart (TImode, pti_reg));
       else
 	{

[ I never received the original mail?  Not the one sent to me directly,
  that is. ]

Subject: [PATCH], rs6000, PR/target 94622, Be more careful with plq for atomic_load<mode>

That is too long...  Also, PR should go at the end, etc., so smth like
Subject: [PATCH] rs6000: Be more careful with plq for atomic_load<mode> (PR94622)
but that is still too long...  write a more succinct subject?  Something
that actually says more than "Be more careful"...  just "Fix" is better
already ;-)

On Mon, Apr 20, 2020 at 04:56:42PM -0500, will schmidt wrote:
> On Mon, 2020-04-20 at 14:00 -0500, Aaron Sawdey via Gcc-patches wrote:
> > For future architecture with prefix instructions, always use plq
> > rather than lq for atomi load of quadword. Then we never have to
> 
> atomic :-)
> 
> > do the doubleword swap on little endian. Before this fix, -mno-pcrel
> > would generate lq with the doubleword swap (which was ok) and -mpcrel
> > would generate plq, also with the doubleword swap, which was wrong.

Was wrong on LE, not on BE.

> > 2020-04-20  Aaron Sawdey  <acsawdey@linux.ibm.com>
> > 
> > 	PR target/94622
> > 	* config/rs6000/sync.md (load_quadpti): Make this have attr
> > prefixed
> > 	if TARGET_PREFIXED.
> 
> I'd rearrange to be
> : Add attr "prefixed" if TARGET_PREFIXED.
> 
> but thats mostly cosmetic, so no big deal.  

It's easier to read and understand ;-)

> > 	(atomic_load<mode>): Do not swap doublewords if TARGET_PREFIXED
> > as
> > 	plq will be used and doesn't need it.
> 
> Ok.   

But it is used on BE as well?  Needs testing (I don't actually mind
either way, but it should be clear and explicit in the code, as well as
in the patch -- just add some words, it does look okay).

> > diff --git a/gcc/config/rs6000/sync.md b/gcc/config/rs6000/sync.md
> > index f27edc77b6a..64dfda6ef75 100644
> > --- a/gcc/config/rs6000/sync.md
> > +++ b/gcc/config/rs6000/sync.md
> > @@ -129,7 +129,10 @@ (define_insn "load_quadpti"
> >    "TARGET_SYNC_TI
> >     && !reg_mentioned_p (operands[0], operands[1])"
> >    "lq %0,%1"
> > -  [(set_attr "type" "load")])
> > +  [(set_attr "type" "load")
> > +   (set_attr "prefixed" (if_then_else (match_test "TARGET_PREFIXED")
> > +                                      (const_string "yes")
> > +                                      (const_string "no")))])

This makes it use plq on BE as well.  This attribute is what *enables*
the "p" character to be output -- so please add a comment here, that we
also do plq on BE?

> >  (define_expand "atomic_load<mode>"
> >    [(set (match_operand:AINT 0 "register_operand")		;;
> > output
> > @@ -162,7 +165,7 @@ (define_expand "atomic_load<mode>"
> > 
> >        emit_insn (gen_load_quadpti (pti_reg, op1));
> > 
> > -      if (WORDS_BIG_ENDIAN)
> > +      if (WORDS_BIG_ENDIAN || TARGET_PREFIXED)
> >  	emit_move_insn (op0, gen_lowpart (TImode, pti_reg));
> >        else
> >  	{

And another comment here I suppose?

Looks fine otherwise :-)


Segher