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

[Segher Boessenkool]

[ 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