Re: [Patch, microblaze]: Add support for swap instructions and reorder option
On 03/05/2013 06:54 AM, David Holsgrove wrote: Hi Michal, -Original Message- From: Michael Eager [mailto:ea...@eagerm.com] Sent: Monday, 4 March 2013 3:37 am To: David Holsgrove Cc: Michael Eager; gcc-patches@gcc.gnu.org; John Williams; Edgar E. Iglesias (edgar.igles...@gmail.com); Vinod Kathail; Vidhumouli Hunsigida; Nagaraju Mekala; Tom Shui Subject: Re: [Patch, microblaze]: Add support for swap instructions and reorder option Committed revision 196415. Thanks for committing. Please submit a patch to update gcc/doc/invoke.texi with -mxl-reorder description. Please find patch attached to this mail which updates the MicroBlaze section of documentation to include -mxl-reorder. I also added -mbig-endian and -mlittle-endian as they were missed in previous patch. Thanks. Committed revision 196470. gcc/ChangeLog: 2013-03-05 David Holsgrove * doc/invoke.texi (MicroBlaze): Add -mbig-endian, -mlittle-endian, -mxl-reorder. Please remember to submit a ChangeLog with patches. -- Michael Eagerea...@eagercon.com 1960 Park Blvd., Palo Alto, CA 94306 650-325-8077
RE: [Patch, microblaze]: Add support for swap instructions and reorder option
Hi Michal, > -Original Message- > From: Michael Eager [mailto:ea...@eagerm.com] > Sent: Monday, 4 March 2013 3:37 am > To: David Holsgrove > Cc: Michael Eager; gcc-patches@gcc.gnu.org; John Williams; Edgar E. Iglesias > (edgar.igles...@gmail.com); Vinod Kathail; Vidhumouli Hunsigida; Nagaraju > Mekala; Tom Shui > Subject: Re: [Patch, microblaze]: Add support for swap instructions and > reorder > option > > Committed revision 196415. Thanks for committing. > > Please submit a patch to update gcc/doc/invoke.texi with -mxl-reorder > description. > Please find patch attached to this mail which updates the MicroBlaze section of documentation to include -mxl-reorder. I also added -mbig-endian and -mlittle-endian as they were missed in previous patch. thanks again, David > > -- > Michael Eager ea...@eagercon.com > 1960 Park Blvd., Palo Alto, CA 94306 650-325-8077 > 0001-Patch-microblaze-Update-gcc-doc-invoke.texi-for-Micr.patch Description: 0001-Patch-microblaze-Update-gcc-doc-invoke.texi-for-Micr.patch
Re: [Patch, microblaze]: Add support for swap instructions and reorder option
On 02/27/2013 04:36 PM, David Holsgrove wrote: -Original Message- From: Michael Eager [mailto:ea...@eagercon.com] Sent: Thursday, 28 February 2013 3:06 am To: David Holsgrove Cc: Michael Eager; gcc-patches@gcc.gnu.org; John Williams; Edgar E. Iglesias (edgar.igles...@gmail.com); Vinod Kathail; Vidhumouli Hunsigida; Nagaraju Mekala; Tom Shui Subject: Re: [Patch, microblaze]: Add support for swap instructions and reorder option The purpose is to avoid issuing a warning for processors before 8.30.a unless the user explicitly specifies -mxl-reorder. Warning the user who explicitly specifies -mxl-reorder with cpu before v8.30.a is the first goal, but we also need to prevent the usage of swap instructions by default if they are not possible to use. I think that the code can be reordered to make it clearer. Replace this + /* TARGET_REORDER initialised as 2 in microblaze.opt, + passing -mxl-reorder sets TARGET_REORDER to 1, + and passing -mno-xl-reorder sets TARGET_REORDER to 0. */ + ver = MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v8.30.a"); + if (ver < 0) +{ +/* MicroBlaze prior to 8.30a didn't have swapb or swaph insns, + so if -mxl-reorder passed, warn and clear TARGET_REORDER. */ +if (TARGET_REORDER == 1) + warning (0, + "-mxl-reorder can be used only with -mcpu=v8.30.a or greater"); +TARGET_REORDER = 0; +} + else if (ver == 0) +{ +/* MicroBlaze v8.30a requires pattern compare for + swapb / swaph insns. */ +if (!TARGET_PATTERN_COMPARE) + TARGET_REORDER = 0; +} With this: /* TARGET_REORDER defaults to 2 if -mxl-reorder not specified. */ if (TARGET_REORDER == 1) { ver = MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v8.30.a"); if (ver < 0) { warning (0, "-mxl-reorder can be used only with -mcpu=v8.30.a or greater"); TARGET_REORDER = 0; } else if ((ver == 0) && !TARGET_PATTERN_COMPARE) { warning (0, "-mxl-reorder requires -mxl-pattern-compare for -mcpu=v8.30.a"); TARGET_REORDER = 0; } } So if we switch to your alternative, the default case (TARGET_REORDER=2) will not be checked for cpu version, or use of TARGET_PATTERN_COMPARE, meaning we emit swap instructions which aren’t valid for these situations. Adjusting your if statement to be; if (TARGET_REORDER) would catch the default case along with explicit use of -mxl-reorder, but then we would also be issuing the warnings for every compilation where cpu is less than v8.30.a, or the user hasn’t passed -mxl-pattern-compare. My attached patch will warn the user if they've explicitly used -mxl-reorder and don’t meet the requirements, and will adjust the default case silently if required. +mxl-reorder +Target Var(TARGET_REORDER) Init(2) +Use reorder instructions (default) Change to +Use reorder instructions (swap and byte reversed load/store) (default) Yes thanks, this is a clearer definition of the intent behind the option. I'll check in the patch with these changes unless you have objections. thanks again for the review, please let me know if this patch is acceptable. David Committed revision 196415. Please submit a patch to update gcc/doc/invoke.texi with -mxl-reorder description. -- Michael Eagerea...@eagercon.com 1960 Park Blvd., Palo Alto, CA 94306 650-325-8077
RE: [Patch, microblaze]: Add support for swap instructions and reorder option
> -Original Message- > From: Michael Eager [mailto:ea...@eagercon.com] > Sent: Thursday, 28 February 2013 3:06 am > To: David Holsgrove > Cc: Michael Eager; gcc-patches@gcc.gnu.org; John Williams; Edgar E. Iglesias > (edgar.igles...@gmail.com); Vinod Kathail; Vidhumouli Hunsigida; Nagaraju > Mekala; Tom Shui > Subject: Re: [Patch, microblaze]: Add support for swap instructions and > reorder > option > > The purpose is to avoid issuing a warning for processors before 8.30.a > unless the user explicitly specifies -mxl-reorder. > Warning the user who explicitly specifies -mxl-reorder with cpu before v8.30.a is the first goal, but we also need to prevent the usage of swap instructions by default if they are not possible to use. > I think that the code can be reordered to make it clearer. > > Replace this > > + /* TARGET_REORDER initialised as 2 in microblaze.opt, > + passing -mxl-reorder sets TARGET_REORDER to 1, > + and passing -mno-xl-reorder sets TARGET_REORDER to 0. */ > + ver = MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v8.30.a"); > + if (ver < 0) > +{ > +/* MicroBlaze prior to 8.30a didn't have swapb or swaph insns, > + so if -mxl-reorder passed, warn and clear TARGET_REORDER. */ > +if (TARGET_REORDER == 1) > + warning (0, > + "-mxl-reorder can be used only with -mcpu=v8.30.a or greater"); > +TARGET_REORDER = 0; > +} > + else if (ver == 0) > +{ > +/* MicroBlaze v8.30a requires pattern compare for > + swapb / swaph insns. */ > +if (!TARGET_PATTERN_COMPARE) > + TARGET_REORDER = 0; > +} > > With this: > > /* TARGET_REORDER defaults to 2 if -mxl-reorder not specified. */ > if (TARGET_REORDER == 1) > { >ver = MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v8.30.a"); >if (ver < 0) >{ > warning (0, "-mxl-reorder can be used only with -mcpu=v8.30.a or > greater"); > TARGET_REORDER = 0; >} >else if ((ver == 0) && !TARGET_PATTERN_COMPARE) >{ > warning (0, "-mxl-reorder requires -mxl-pattern-compare for > -mcpu=v8.30.a"); > TARGET_REORDER = 0; >} > } So if we switch to your alternative, the default case (TARGET_REORDER=2) will not be checked for cpu version, or use of TARGET_PATTERN_COMPARE, meaning we emit swap instructions which aren’t valid for these situations. Adjusting your if statement to be; if (TARGET_REORDER) would catch the default case along with explicit use of -mxl-reorder, but then we would also be issuing the warnings for every compilation where cpu is less than v8.30.a, or the user hasn’t passed -mxl-pattern-compare. My attached patch will warn the user if they've explicitly used -mxl-reorder and don’t meet the requirements, and will adjust the default case silently if required. > +mxl-reorder > +Target Var(TARGET_REORDER) Init(2) > +Use reorder instructions (default) > > Change to > +Use reorder instructions (swap and byte reversed load/store) (default) > Yes thanks, this is a clearer definition of the intent behind the option. > > I'll check in the patch with these changes unless you have objections. > thanks again for the review, please let me know if this patch is acceptable. David > > -- > Michael Eager ea...@eagercon.com > 1960 Park Blvd., Palo Alto, CA 94306 650-325-8077 0001-microblaze-add-support-for-swap-instructions-and-reo.patch Description: 0001-microblaze-add-support-for-swap-instructions-and-reo.patch
Re: [Patch, microblaze]: Add support for swap instructions and reorder option
On 02/27/2013 06:42 AM, David Holsgrove wrote: Hi Michael, Thanks for the review, please find comments inline below. -Original Message- From: Michael Eager [mailto:ea...@eagerm.com] Sent: Wednesday, 27 February 2013 3:50 am To: David Holsgrove Cc: gcc-patches@gcc.gnu.org; Michael Eager (ea...@eagercon.com); John Williams; Edgar E. Iglesias (edgar.igles...@gmail.com); Vinod Kathail; Vidhumouli Hunsigida; Nagaraju Mekala; Tom Shui Subject: Re: [Patch, microblaze]: Add support for swap instructions and reorder option On 02/10/2013 10:39 PM, David Holsgrove wrote: Add support for swap instructions and reorder option swapb and swaph instructions are introduced in microblaze cpu (mcpu) v8.30a, but have an undocumented dependence on -mxl-pattern-compare being set. The conditions for their use are; mcpu < 8.30a; no swap insns, use of -mxl-reorder produces warning and ignored mcpu == 8.30a and -mxl-pattern-compare specified; and if -mno-xl-reorder not specified, then swap insns allowed mcpu > 8.30a; if -mno-xl-reorder not specified, then swap insns allowed The use of separate -mxl-reorder / -mno-xl-reorder options was to be able to discriminate between the three states; 1) user passes nothing 2) user passes -mxl-reorder 3) user passes -mno-xl-reorder I've reworked the patch to instead define the -mxl-reorder option only, but with an initial value of 2 (which is similar to arm's option -mfix-cortex-m3-ldrd) GCC option handling will then create the -mno-xl-reorder option for us and set TARGET_REORDER to 0 if -mno-xl-reorder used, or 1 if -mxl-reorder passed. This allows detection and handling of the three separate cases above. The purpose is to avoid issuing a warning for processors before 8.30.a unless the user explicitly specifies -mxl-reorder. I think that the code can be reordered to make it clearer. Replace this + /* TARGET_REORDER initialised as 2 in microblaze.opt, + passing -mxl-reorder sets TARGET_REORDER to 1, + and passing -mno-xl-reorder sets TARGET_REORDER to 0. */ + ver = MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v8.30.a"); + if (ver < 0) +{ +/* MicroBlaze prior to 8.30a didn't have swapb or swaph insns, + so if -mxl-reorder passed, warn and clear TARGET_REORDER. */ +if (TARGET_REORDER == 1) + warning (0, + "-mxl-reorder can be used only with -mcpu=v8.30.a or greater"); +TARGET_REORDER = 0; +} + else if (ver == 0) +{ +/* MicroBlaze v8.30a requires pattern compare for + swapb / swaph insns. */ +if (!TARGET_PATTERN_COMPARE) + TARGET_REORDER = 0; +} With this: /* TARGET_REORDER defaults to 2 if -mxl-reorder not specified. */ if (TARGET_REORDER == 1) { ver = MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v8.30.a"); if (ver < 0) { warning (0, "-mxl-reorder can be used only with -mcpu=v8.30.a or greater"); TARGET_REORDER = 0; } else if ((ver == 0) && !TARGET_PATTERN_COMPARE) { warning (0, "-mxl-reorder requires -mxl-pattern-compare for -mcpu=v8.30.a"); TARGET_REORDER = 0; } } How does the name of the option (-mxl-reorder) relate to using swap instructions? Nothing is being reordered. What are "reorder" instructions? How about -mxl-swap, similar to -mxl-pattern-compare or -mxl-float-sqrt? The choice of the name for the option (-mxl-reorder) is driven by its use in Xilinx's EDK I believe, and joins the reverse load / reverse store instructions in a 'reorder' instructions group; (http://www.xilinx.com/support/documentation/sw_manuals/xilinx14_4/mb_ref_guide.pdf#page=135) While I think I understand the viewpoint, that data transfer from/to memory is reordered to change endianness on load or store, I think that this terminology is confusing. Swap doesn't transfer data from memory. Where is the reverse load/store support? Refactor to eliminate duplicated code. Why set the MASK_REORDER flag in target_flags if this is never used? Removed unused MASK_REORDER, and refactored this block of logic. Default is to emit swap instructions (TARGET_REORDER != 0), unless user passes -mno-xl-reorder, or microblaze_option_override forces TARGET_REORDER to 0. Thanks. Options processing will set this automatically since you have mxl-reorder Target RejectNegative Mask(REORDER) I've attached an updated version of this patch. Please let me know if you have any concerns about it. +mxl-reorder +Target Var(TARGET_REORDER) Init(2) +Use reorder instructions (default) Change to +Use reorder instructions (swap and byte reversed load/store) (default) I'll check in the patch with these changes unless you have objections. -- Michael Eagerea...@eagercon.com 1960 Park Blvd., Palo Alto, CA 94306 650-325-8077
RE: [Patch, microblaze]: Add support for swap instructions and reorder option
Hi Michael, Thanks for the review, please find comments inline below. > -Original Message- > From: Michael Eager [mailto:ea...@eagerm.com] > Sent: Wednesday, 27 February 2013 3:50 am > To: David Holsgrove > Cc: gcc-patches@gcc.gnu.org; Michael Eager (ea...@eagercon.com); John > Williams; Edgar E. Iglesias (edgar.igles...@gmail.com); Vinod Kathail; > Vidhumouli > Hunsigida; Nagaraju Mekala; Tom Shui > Subject: Re: [Patch, microblaze]: Add support for swap instructions and > reorder > option > > On 02/10/2013 10:39 PM, David Holsgrove wrote: > > Add support for swap instructions and reorder option > > > > swapb and swaph instructions are introduced in microblaze cpu (mcpu) v8.30a, > > but have an undocumented dependence on -mxl-pattern-compare being set. > > > > The conditions for their use are; > > > > mcpu < 8.30a; no swap insns, use of -mxl-reorder produces warning > > and ignored > > > > mcpu == 8.30a and -mxl-pattern-compare specified; > > and if -mno-xl-reorder not specified, then swap insns allowed > > > > mcpu > 8.30a; > > if -mno-xl-reorder not specified, then swap insns allowed > > > > Changelog > > > > 2013-02-11 David Holsgrove > > Is this correct? > Changelog amended in light of adjustments to patch. > > > >* config/microblaze/microblaze.c: microblaze_has_swap = 0 > > Add version check for v8.30.a to enable microblaze_has_swap > >* config/microblaze/microblaze.h: Add TARGET_HAS_SWAP > >* config/microblaze/microblaze.md: New bswapsi2 and bswaphi2 > > instructions > >* config/microblaze/microblaze.opt: New options -mxl-reorder > > and -mno-xl-reorder > > Don't specify both -mxl-reorder and -mno-xl-reorder as options. > Don't specify a "no" version with the RejectNegative option. > GCC option handling already process the "no" prefix automatically. > > There is no need to have both TARGET_REORDER and TARGET_NOREORDER > since they have exactly the same information. Define only TARGET_REORDER. > The use of separate -mxl-reorder / -mno-xl-reorder options was to be able to discriminate between the three states; 1) user passes nothing 2) user passes -mxl-reorder 3) user passes -mno-xl-reorder I've reworked the patch to instead define the -mxl-reorder option only, but with an initial value of 2 (which is similar to arm's option -mfix-cortex-m3-ldrd) GCC option handling will then create the -mno-xl-reorder option for us and set TARGET_REORDER to 0 if -mno-xl-reorder used, or 1 if -mxl-reorder passed. This allows detection and handling of the three separate cases above. > How does the name of the option (-mxl-reorder) relate to using swap > instructions? Nothing is being reordered. What are "reorder" instructions? > How about -mxl-swap, similar to -mxl-pattern-compare or -mxl-float-sqrt? > The choice of the name for the option (-mxl-reorder) is driven by its use in Xilinx's EDK I believe, and joins the reverse load / reverse store instructions in a 'reorder' instructions group; (http://www.xilinx.com/support/documentation/sw_manuals/xilinx14_4/mb_ref_guide.pdf#page=135) > > + else if (ver == 0) > +{ > +if (!TARGET_NO_REORDER) > + { > +target_flags |= MASK_REORDER; > +/* MicroBlaze v8.30a has an undocumented dependency on > + pattern compare for swapb / swaph insns. */ > +if (TARGET_PATTERN_COMPARE) > + microblaze_has_swap = 1; > + } > +} > + else > +{ > +if (!TARGET_NO_REORDER) > + { > +target_flags |= MASK_REORDER; > +/* Microblaze versions greater than v8.30a will be able to use > + swapb / swaph without pattern compare */ > +microblaze_has_swap = 1; > + } > +} > + > > Refactor to eliminate duplicated code. > > Why set the MASK_REORDER flag in target_flags if this is never used? Removed unused MASK_REORDER, and refactored this block of logic. Default is to emit swap instructions (TARGET_REORDER != 0), unless user passes -mno-xl-reorder, or microblaze_option_override forces TARGET_REORDER to 0. > Options processing will set this automatically since you have >mxl-reorder >Target RejectNegative Mask(REORDER) > I've attached an updated version of this patch. Please let me know if you have any concerns about it. The new Changelog entry would be as follows; Changelog 2013-02-27 David Holsgrove * gcc/config/microblaze/microblaze.c: Check mcpu, pcmp requirement and set TARGET_REORDER to 0 if not met. * gcc/config/mi
Re: [Patch, microblaze]: Add support for swap instructions and reorder option
On 02/10/2013 10:39 PM, David Holsgrove wrote: Add support for swap instructions and reorder option swapb and swaph instructions are introduced in microblaze cpu (mcpu) v8.30a, but have an undocumented dependence on -mxl-pattern-compare being set. The conditions for their use are; mcpu < 8.30a; no swap insns, use of -mxl-reorder produces warning and ignored mcpu == 8.30a and -mxl-pattern-compare specified; and if -mno-xl-reorder not specified, then swap insns allowed mcpu > 8.30a; if -mno-xl-reorder not specified, then swap insns allowed Changelog 2013-02-11 David Holsgrove Is this correct? * config/microblaze/microblaze.c: microblaze_has_swap = 0 Add version check for v8.30.a to enable microblaze_has_swap * config/microblaze/microblaze.h: Add TARGET_HAS_SWAP * config/microblaze/microblaze.md: New bswapsi2 and bswaphi2 instructions * config/microblaze/microblaze.opt: New options -mxl-reorder and -mno-xl-reorder Don't specify both -mxl-reorder and -mno-xl-reorder as options. Don't specify a "no" version with the RejectNegative option. GCC option handling already process the "no" prefix automatically. There is no need to have both TARGET_REORDER and TARGET_NOREORDER since they have exactly the same information. Define only TARGET_REORDER. How does the name of the option (-mxl-reorder) relate to using swap instructions? Nothing is being reordered. What are "reorder" instructions? How about -mxl-swap, similar to -mxl-pattern-compare or -mxl-float-sqrt? + else if (ver == 0) +{ +if (!TARGET_NO_REORDER) + { +target_flags |= MASK_REORDER; +/* MicroBlaze v8.30a has an undocumented dependency on + pattern compare for swapb / swaph insns. */ +if (TARGET_PATTERN_COMPARE) + microblaze_has_swap = 1; + } +} + else +{ +if (!TARGET_NO_REORDER) + { +target_flags |= MASK_REORDER; +/* Microblaze versions greater than v8.30a will be able to use + swapb / swaph without pattern compare */ +microblaze_has_swap = 1; + } +} + Refactor to eliminate duplicated code. Why set the MASK_REORDER flag in target_flags if this is never used? Options processing will set this automatically since you have mxl-reorder Target RejectNegative Mask(REORDER) -- Michael Eagerea...@eagercon.com 1960 Park Blvd., Palo Alto, CA 94306 650-325-8077