diff mbox series

V5, #2 of 15: Fix prefixed instruction length for some 128-bit types

Message ID 20191009195601.GB2063@ibm-toto.the-meissners.org
State New
Headers show
Series V5, #2 of 15: Fix prefixed instruction length for some 128-bit types | expand

Commit Message

Michael Meissner Oct. 9, 2019, 7:56 p.m. UTC 
This patch fixes the prefixed and non-prefixed instruction sizes for the
128-bit types that aren't loaded into 128-bit vectors (TDmode, TFmode, IFmode,
PTImode).

Along with the other patches, I have done bootstraps on a little endian power8
system, and there were no regressions in the test suite.  Can I check this into
the trunk?

2019-10-08  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/rs6000.md (mov<mode>_64bit_dm): Set prefixed and
	non-prefixed length.
	(movtd_64bit_nodm): Set prefixed and non-prefixed length.
	(mov<mode>_ppc64): Set prefixed and non-prefixed length.

Comments

Segher Boessenkool Oct. 10, 2019, 10:02 p.m. UTC | #1 
On Wed, Oct 09, 2019 at 03:56:01PM -0400, Michael Meissner wrote:
> This patch fixes the prefixed and non-prefixed instruction sizes for the
> 128-bit types that aren't loaded into 128-bit vectors (TDmode, TFmode, IFmode,
> PTImode).

> 2019-10-08  Michael Meissner  <meissner@linux.ibm.com>
> 
> 	* config/rs6000/rs6000.md (mov<mode>_64bit_dm): Set prefixed and
> 	non-prefixed length.
> 	(movtd_64bit_nodm): Set prefixed and non-prefixed length.
> 	(mov<mode>_ppc64): Set prefixed and non-prefixed length.

Please also note the patterns you reformatted.  (Just "Reformat." is
enough of course).

>  (define_insn_and_split "*movtd_64bit_nodm"
>    [(set (match_operand:TD 0 "nonimmediate_operand" "=m,d,d,Y,r,r")
> @@ -7786,8 +7790,12 @@ (define_insn_and_split "*movtd_64bit_nod
>    "#"
>    "&& reload_completed"
>    [(pc)]
> -{ rs6000_split_multireg_move (operands[0], operands[1]); DONE; }
> -  [(set_attr "length" "8,8,8,12,12,8")])
> +{
> +  rs6000_split_multireg_move (operands[0], operands[1]);
> +  DONE;
> +}
> +  [(set_attr "non_prefixed_length" "8")
> +   (set_attr "prefixed_length" "20")])

It used to be 8,8,8,12,12,8 before.  Was that in error?  Please explain.


Segher
Michael Meissner Oct. 14, 2019, 9:12 p.m. UTC | #2 
On Thu, Oct 10, 2019 at 05:02:09PM -0500, Segher Boessenkool wrote:
> On Wed, Oct 09, 2019 at 03:56:01PM -0400, Michael Meissner wrote:
> > This patch fixes the prefixed and non-prefixed instruction sizes for the
> > 128-bit types that aren't loaded into 128-bit vectors (TDmode, TFmode, IFmode,
> > PTImode).
> 
> > 2019-10-08  Michael Meissner  <meissner@linux.ibm.com>
> > 
> > 	* config/rs6000/rs6000.md (mov<mode>_64bit_dm): Set prefixed and
> > 	non-prefixed length.
> > 	(movtd_64bit_nodm): Set prefixed and non-prefixed length.
> > 	(mov<mode>_ppc64): Set prefixed and non-prefixed length.
> 
> Please also note the patterns you reformatted.  (Just "Reformat." is
> enough of course).

Ok.

> >  (define_insn_and_split "*movtd_64bit_nodm"
> >    [(set (match_operand:TD 0 "nonimmediate_operand" "=m,d,d,Y,r,r")
> > @@ -7786,8 +7790,12 @@ (define_insn_and_split "*movtd_64bit_nod
> >    "#"
> >    "&& reload_completed"
> >    [(pc)]
> > -{ rs6000_split_multireg_move (operands[0], operands[1]); DONE; }
> > -  [(set_attr "length" "8,8,8,12,12,8")])
> > +{
> > +  rs6000_split_multireg_move (operands[0], operands[1]);
> > +  DONE;
> > +}
> > +  [(set_attr "non_prefixed_length" "8")
> > +   (set_attr "prefixed_length" "20")])
> 
> It used to be 8,8,8,12,12,8 before.  Was that in error?  Please explain.

We've had this discussion before, but I didn't update the ChangeLog.

The two cases for 12 bytes (i.e. 3 insns) are r->Y and Y->r constaints.  Y
matches DS offsets (i.e. bottom 2 bits non-zero) for traditional instructions.
In looking at rs6000_split_multireg_move, the only way I can see that 3
instructions would be generated would be if pre_modify/pre_update/etc. were
generated.  But we don't allow pre_modify on larger types.

But if you think there might be a case where it does generate 3 instructions, I
can modify it to use 8,8,8,12,12,8 for the non-prefixed case, and
20,20,20,24,24,20 for the prefixed case.
Segher Boessenkool Oct. 14, 2019, 10:12 p.m. UTC | #3 
On Mon, Oct 14, 2019 at 05:12:56PM -0400, Michael Meissner wrote:
> On Thu, Oct 10, 2019 at 05:02:09PM -0500, Segher Boessenkool wrote:
> > > @@ -7786,8 +7790,12 @@ (define_insn_and_split "*movtd_64bit_nod
> > >    "#"
> > >    "&& reload_completed"
> > >    [(pc)]
> > > -{ rs6000_split_multireg_move (operands[0], operands[1]); DONE; }
> > > -  [(set_attr "length" "8,8,8,12,12,8")])
> > > +{
> > > +  rs6000_split_multireg_move (operands[0], operands[1]);
> > > +  DONE;
> > > +}
> > > +  [(set_attr "non_prefixed_length" "8")
> > > +   (set_attr "prefixed_length" "20")])
> > 
> > It used to be 8,8,8,12,12,8 before.  Was that in error?  Please explain.
> 
> We've had this discussion before, but I didn't update the ChangeLog.
> 
> The two cases for 12 bytes (i.e. 3 insns) are r->Y and Y->r constaints.  Y
> matches DS offsets (i.e. bottom 2 bits non-zero) for traditional instructions.
> In looking at rs6000_split_multireg_move, the only way I can see that 3
> instructions would be generated would be if pre_modify/pre_update/etc. were
> generated.  But we don't allow pre_modify on larger types.

So do this as a separate change, first, please?

If it cannot happen, please also add an assert (somewhere early, before
doing anything else with the automodifies), an remove any now-dead code
(if there is any).

> But if you think there might be a case where it does generate 3 instructions, I
> can modify it to use 8,8,8,12,12,8 for the non-prefixed case, and
> 20,20,20,24,24,20 for the prefixed case.

I think you are right, but it's not obvious, and it warrants a separate
patch.  That way, we can easily bisect it if some problem manifests, etc.
Thanks,


Segher
diff mbox series

Patch

Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md	(revision 276713)
+++ gcc/config/rs6000/rs6000.md	(working copy)
@@ -7773,9 +7773,13 @@  (define_insn_and_split "*mov<mode>_64bit
   "#"
   "&& reload_completed"
   [(pc)]
-{ rs6000_split_multireg_move (operands[0], operands[1]); DONE; }
-  [(set_attr "length" "8")
-   (set_attr "isa" "*,*,*,*,*,*,*,*,p8v,p8v")])
+{
+  rs6000_split_multireg_move (operands[0], operands[1]);
+  DONE;
+}
+  [(set_attr "isa" "*,*,*,*,*,*,*,*,p8v,p8v")
+   (set_attr "non_prefixed_length" "8")
+   (set_attr "prefixed_length" "20")])
 
 (define_insn_and_split "*movtd_64bit_nodm"
   [(set (match_operand:TD 0 "nonimmediate_operand" "=m,d,d,Y,r,r")
@@ -7786,8 +7790,12 @@  (define_insn_and_split "*movtd_64bit_nod
   "#"
   "&& reload_completed"
   [(pc)]
-{ rs6000_split_multireg_move (operands[0], operands[1]); DONE; }
-  [(set_attr "length" "8,8,8,12,12,8")])
+{
+  rs6000_split_multireg_move (operands[0], operands[1]);
+  DONE;
+}
+  [(set_attr "non_prefixed_length" "8")
+   (set_attr "prefixed_length" "20")])
 
 (define_insn_and_split "*mov<mode>_32bit"
   [(set (match_operand:FMOVE128_FPR 0 "nonimmediate_operand" "=m,d,d,d,Y,r,r")
@@ -8984,7 +8992,8 @@  (define_insn "*mov<mode>_ppc64"
   return rs6000_output_move_128bit (operands);
 }
   [(set_attr "type" "store,store,load,load,*,*")
-   (set_attr "length" "8")])
+   (set_attr "prefixed_length" "20")
+   (set_attr "non_prefixed_length" "8")])
 
 (define_split
   [(set (match_operand:TI2 0 "int_reg_operand")