| Message ID | EB4625145972F94C9680D8CADD65161578E9323A@satlexdag04.amd.com |
|---|---|
| State | New |
| Headers | show |
On 30 October 2014 08:54, Gopalasubramanian, Ganesh <Ganesh.Gopalasubramanian@amd.com> wrote: > 2014-10-30 Ganesh Gopalasubramanian <Ganesh.Gopalasubramanian@amd.com> Check the whitespace in your ChangeLog line. > * config/arm/types.md (define_attr "type"): Add prefetch. The existing schedulers use 'load1'. We can of course split that into two introducing "prefetch" and update all of the existing schedulers to reflect the change. However I suggest we do that as a separate activity when someone actually needs the distinction, note this change will require updating the schedulers for both ARM and AArch64 backends not just those relevant to AArch64. For this prefetch patch I suggest we go with the existing "load1". The inline patch has been munged by your mailer, I tried applying the patch to my tree but it is full of escape sequences. Can you either fix your mailer or submit patches as attachments? > diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md > index 74b554e..12a3f170 100644 > --- a/gcc/config/aarch64/aarch64.md > +++ b/gcc/config/aarch64/aarch64.md > @@ -320,6 +320,38 @@ > [(set_attr "type" "no_insn")] > ) > > + > +(define_insn "prefetch" > + [(prefetch (match_operand:DI 0 "address_operand" "r") > + (match_operand:QI 1 "const_int_operand" "") > + (match_operand:QI 2 "const_int_operand" ""))] > + "" > + "* > +{ Use {} instead of "*{, then all of the extra quoting in the C below goes away. > + const char * pftype[2][10] > + = { {\"PLDL1STRM\", \"PLDL3KEEP\", \"PLDL2KEEP\", \"PLDL1KEEP\"}, > + {\"PSTL1STRM\", \"PSTL3KEEP\", \"PSTL2KEEP\", \"PSTL1KEEP\"}, > + }; > + > + int locality = INTVAL (operands[2]); > + char pattern[100]; > + > + gcc_assert (IN_RANGE (locality, 0, 3)); > + > + strcpy (pattern, \"prfm\\t\"); > + strcat (pattern, (const char*)pftype[INTVAL(operands[1])][locality]); > + strcat (pattern, \", %a0\"); Use sprintf() rather that multiple calls to cpy and cat. I suspect the cast in front of pftype is superflous? > + > + output_asm_insn (pattern, > + operands); Unnecessary line break. Cheers /Marcus
> For this prefetch patch I suggest we go with the existing "load1". I have removed the changes done in types.md. > The inline patch has been munged by your mailer, I tried applying the patch to my tree but it is full of escape sequences. Can you either fix your mailer or submit patches as attachments? I am attaching the revised patch. > Check the whitespace in your ChangeLog line. Changelog entry is also embedded in the attachment. Regards Ganesh
On 11/14/2014 09:11 PM, Gopalasubramanian, Ganesh wrote: > + const char * pftype[2][10] > + = { {"PLDL1STRM", "PLDL3KEEP", "PLDL2KEEP", "PLDL1KEEP"}, > + {"PSTL1STRM", "PSTL3KEEP", "PSTL2KEEP", "PSTL1KEEP"}, > + }; The array should be static const char * const pftype[2][4] I've no idea where you got that "10" from, espectially since... > + gcc_assert (IN_RANGE (locality, 0, 3)); ... you've constrained it right here. > + sprintf (pattern, "prfm\\t%s, %%a0", > + pftype[INTVAL(operands[1])][locality]); There's no point in the buffer or the sprintf. The text is short enough to repeat whole pattern in the array: static const char * const pftype[2][4] = { { "prfm\\tPLDL1STRM, %a0", ... }, { "prfm\\tPSTL1STRM, %a0", ... } }; ... return pftype[INTVAL(operands[1])][INTVAL(operands[2])]; r~
> There's no point in the buffer or the sprintf. > The text is short enough to repeat whole pattern in the array: Updated the patch for the above suggestions. Is it ok for upstream? Regards Ganesh
Please ignore the previous patch sent. The attachment was wrong. > There's no point in the buffer or the sprintf. > The text is short enough to repeat whole pattern in the array: Updated the patch for the above suggestions. make -k check RUNTESTFLAGS="execute.exp compile.exp dg.exp" passes. Is it ok for upstream? Regards Ganesh
On 12/01/2014 05:48 PM, Gopalasubramanian, Ganesh wrote: > +2014-12-01 Ganesh Gopalasubramanian <Ganesh.Gopalasubramanian@amd.com> > + > + * config/aarch64/aarch64.md (define_insn "prefetch"): New. > + Looks good to me. r~
On 1 December 2014 at 07:48, Gopalasubramanian, Ganesh <Ganesh.Gopalasubramanian@amd.com> wrote: > Please ignore the previous patch sent. The attachment was wrong. > >> There's no point in the buffer or the sprintf. >> The text is short enough to repeat whole pattern in the array: > > Updated the patch for the above suggestions. > make -k check RUNTESTFLAGS="execute.exp compile.exp dg.exp" passes. > > Is it ok for upstream? OK, Thanks /Marcus
On Tue, Nov 11, 2014 at 6:47 AM, Marcus Shawcroft <marcus.shawcroft@gmail.com> wrote: > On 30 October 2014 08:54, Gopalasubramanian, Ganesh > <Ganesh.Gopalasubramanian@amd.com> wrote: > >> 2014-10-30 Ganesh Gopalasubramanian <Ganesh.Gopalasubramanian@amd.com> > > Check the whitespace in your ChangeLog line. > >> * config/arm/types.md (define_attr "type"): Add prefetch. > > The existing schedulers use 'load1'. We can of course split that into > two introducing "prefetch" and update all of the existing schedulers > to reflect the change. However I suggest we do that as a separate > activity when someone actually needs the distinction, note this change > will require updating the schedulers for both ARM and AArch64 backends > not just those relevant to AArch64. For this prefetch patch I suggest > we go with the existing "load1". I will need this change for ThunderX schedule. The Pref instruction is single issued while load1 can be dual issued. Thanks, Andrew > > The inline patch has been munged by your mailer, I tried applying the > patch to my tree but it is full of escape sequences. Can you either > fix your mailer or submit patches as attachments? > >> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md >> index 74b554e..12a3f170 100644 >> --- a/gcc/config/aarch64/aarch64.md >> +++ b/gcc/config/aarch64/aarch64.md >> @@ -320,6 +320,38 @@ >> [(set_attr "type" "no_insn")] >> ) >> >> + >> +(define_insn "prefetch" >> + [(prefetch (match_operand:DI 0 "address_operand" "r") >> + (match_operand:QI 1 "const_int_operand" "") >> + (match_operand:QI 2 "const_int_operand" ""))] >> + "" > >> + "* >> +{ > > Use {} instead of "*{, then all of the extra quoting in the C below goes away. > >> + const char * pftype[2][10] >> + = { {\"PLDL1STRM\", \"PLDL3KEEP\", \"PLDL2KEEP\", \"PLDL1KEEP\"}, >> + {\"PSTL1STRM\", \"PSTL3KEEP\", \"PSTL2KEEP\", \"PSTL1KEEP\"}, >> + }; >> + >> + int locality = INTVAL (operands[2]); >> + char pattern[100]; >> + >> + gcc_assert (IN_RANGE (locality, 0, 3)); >> + >> + strcpy (pattern, \"prfm\\t\"); >> + strcat (pattern, (const char*)pftype[INTVAL(operands[1])][locality]); >> + strcat (pattern, \", %a0\"); > > Use sprintf() rather that multiple calls to cpy and cat. I suspect > the cast in front of pftype is superflous? > >> + >> + output_asm_insn (pattern, >> + operands); > > Unnecessary line break. > > Cheers > /Marcus
On 11 January 2015 at 02:37, Andrew Pinski <pinskia@gmail.com> wrote: > On Tue, Nov 11, 2014 at 6:47 AM, Marcus Shawcroft > <marcus.shawcroft@gmail.com> wrote: >> On 30 October 2014 08:54, Gopalasubramanian, Ganesh >> <Ganesh.Gopalasubramanian@amd.com> wrote: >> >>> 2014-10-30 Ganesh Gopalasubramanian <Ganesh.Gopalasubramanian@amd.com> >> >> Check the whitespace in your ChangeLog line. >> >>> * config/arm/types.md (define_attr "type"): Add prefetch. >> >> The existing schedulers use 'load1'. We can of course split that into >> two introducing "prefetch" and update all of the existing schedulers >> to reflect the change. However I suggest we do that as a separate >> activity when someone actually needs the distinction, note this change >> will require updating the schedulers for both ARM and AArch64 backends >> not just those relevant to AArch64. For this prefetch patch I suggest >> we go with the existing "load1". > > I will need this change for ThunderX schedule. The Pref instruction > is single issued while load1 can be dual issued. Hi https://gcc.gnu.org/ml/gcc-patches/2015-01/msg00802.html Philipp when you deal with Ramana's request above to split load1->load1/prefetch in the existing schedulers I suggest you also split it in aarch64/thunderx.md in order to retain existing behaviour. Andrew can then follow up add the "right" behaviour when he is ready. Andrew OK ? Cheers /Marcus
On Tue, Jan 13, 2015 at 6:13 AM, Marcus Shawcroft <marcus.shawcroft@gmail.com> wrote: > On 11 January 2015 at 02:37, Andrew Pinski <pinskia@gmail.com> wrote: >> On Tue, Nov 11, 2014 at 6:47 AM, Marcus Shawcroft >> <marcus.shawcroft@gmail.com> wrote: >>> On 30 October 2014 08:54, Gopalasubramanian, Ganesh >>> <Ganesh.Gopalasubramanian@amd.com> wrote: >>> >>>> 2014-10-30 Ganesh Gopalasubramanian <Ganesh.Gopalasubramanian@amd.com> >>> >>> Check the whitespace in your ChangeLog line. >>> >>>> * config/arm/types.md (define_attr "type"): Add prefetch. >>> >>> The existing schedulers use 'load1'. We can of course split that into >>> two introducing "prefetch" and update all of the existing schedulers >>> to reflect the change. However I suggest we do that as a separate >>> activity when someone actually needs the distinction, note this change >>> will require updating the schedulers for both ARM and AArch64 backends >>> not just those relevant to AArch64. For this prefetch patch I suggest >>> we go with the existing "load1". >> >> I will need this change for ThunderX schedule. The Pref instruction >> is single issued while load1 can be dual issued. > > Hi > > https://gcc.gnu.org/ml/gcc-patches/2015-01/msg00802.html > > Philipp when you deal with Ramana's request above to split > load1->load1/prefetch in the existing schedulers I suggest you also > split it in aarch64/thunderx.md in order to retain existing behaviour. > Andrew can then follow up add the "right" behaviour when he is ready. > Andrew OK ? Yes that sounds ok to me. I was going to submit an update to thunderx.md file this week anyways. Thanks, Andrew > > Cheers > /Marcus
Great. I should have an update patch-set ready & tested later tonight. Best, Phil. > On 13 Jan 2015, at 15:18, Andrew Pinski <pinskia@gmail.com> wrote: > > On Tue, Jan 13, 2015 at 6:13 AM, Marcus Shawcroft > <marcus.shawcroft@gmail.com> wrote: >> On 11 January 2015 at 02:37, Andrew Pinski <pinskia@gmail.com> wrote: >>> On Tue, Nov 11, 2014 at 6:47 AM, Marcus Shawcroft >>> <marcus.shawcroft@gmail.com> wrote: >>>> On 30 October 2014 08:54, Gopalasubramanian, Ganesh >>>> <Ganesh.Gopalasubramanian@amd.com> wrote: >>>> >>>>> 2014-10-30 Ganesh Gopalasubramanian <Ganesh.Gopalasubramanian@amd.com> >>>> >>>> Check the whitespace in your ChangeLog line. >>>> >>>>> * config/arm/types.md (define_attr "type"): Add prefetch. >>>> >>>> The existing schedulers use 'load1'. We can of course split that into >>>> two introducing "prefetch" and update all of the existing schedulers >>>> to reflect the change. However I suggest we do that as a separate >>>> activity when someone actually needs the distinction, note this change >>>> will require updating the schedulers for both ARM and AArch64 backends >>>> not just those relevant to AArch64. For this prefetch patch I suggest >>>> we go with the existing "load1". >>> >>> I will need this change for ThunderX schedule. The Pref instruction >>> is single issued while load1 can be dual issued. >> >> Hi >> >> https://gcc.gnu.org/ml/gcc-patches/2015-01/msg00802.html >> >> Philipp when you deal with Ramana's request above to split >> load1->load1/prefetch in the existing schedulers I suggest you also >> split it in aarch64/thunderx.md in order to retain existing behaviour. >> Andrew can then follow up add the "right" behaviour when he is ready. >> Andrew OK ? > > Yes that sounds ok to me. I was going to submit an update to > thunderx.md file this week anyways. > > Thanks, > Andrew > > >> >> Cheers >> /Marcus
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index 74b554e..12a3f170 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -320,6 +320,38 @@ [(set_attr "type" "no_insn")] ) + +(define_insn "prefetch" + [(prefetch (match_operand:DI 0 "address_operand" "r") + (match_operand:QI 1 "const_int_operand" "") + (match_operand:QI 2 "const_int_operand" ""))] + "" + "* +{ + const char * pftype[2][10] + = { {\"PLDL1STRM\", \"PLDL3KEEP\", \"PLDL2KEEP\", \"PLDL1KEEP\"}, + {\"PSTL1STRM\", \"PSTL3KEEP\", \"PSTL2KEEP\", \"PSTL1KEEP\"}, + }; + + int locality = INTVAL (operands[2]); + char pattern[100]; + + gcc_assert (IN_RANGE (locality, 0, 3)); + + strcpy (pattern, \"prfm\\t\"); + strcat (pattern, (const char*)pftype[INTVAL(operands[1])][locality]); + strcat (pattern, \", %a0\"); + + output_asm_insn (pattern, + operands); + + return \"\"; + +}" + [(set_attr "type" "prefetch")] +) + (define_insn "trap" [(trap_if (const_int 1) (const_int 8))] "" diff --git a/gcc/config/arm/types.md b/gcc/config/arm/types.md index c1151f5..8b4b7a1 100644 --- a/gcc/config/arm/types.md +++ b/gcc/config/arm/types.md @@ -118,6 +118,7 @@ ; mvn_shift_reg inverting move instruction, shifted operand by a register. ; no_insn an insn which does not represent an instruction in the ; final output, thus having no impact on scheduling. +; prefetch a prefetch instruction ; rbit reverse bits. ; rev reverse bytes. ; sdiv signed division. @@ -556,6 +557,7 @@ call,\ clz,\ no_insn,\ + prefetch,\ csel,\ crc,\