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