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 Eager ea...@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306 650-325-8077
