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 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 david.holsgr...@xilinx.com * 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
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
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
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
-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
