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 <david.holsgr...@xilinx.com> > > 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 <david.holsgr...@xilinx.com> * gcc/config/microblaze/microblaze.c: Check mcpu, pcmp requirement and set TARGET_REORDER to 0 if not met. * gcc/config/microblaze/microblaze.h: Add -mxl-reorder to DRIVER_SELF_SPECS * gcc/config/microblaze/microblaze.md: New bswapsi2 and bswaphi2 instructions emitted if TARGET_REORDER * gcc/config/microblaze/microblaze.opt: New option -mxl-reorder set to 1 or 0 for -m/-mno case, but initialises as 2 to detect default use case separately thanks, 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
