[Patch, microblaze]: Add support for swap instructions and reorder option

[Patch, microblaze]: Add support for swap instructions and reorder option

[David Holsgrove]

[-- Attachment #1: Type: text/plain, Size: 951 bytes --]

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.holsgrove@xilinx.com>

  * 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


[-- Attachment #2: 0007-Gcc-Added-swapb-and-swaph-instructions-and-mxl-reord.patch --]
[-- Type: application/octet-stream, Size: 6150 bytes --]

From ba9181676faaa1ad3d9e8bcb4cebe239fdbf05a2 Mon Sep 17 00:00:00 2001
From: David Holsgrove <david.holsgrove@xilix.com>
Date: Tue, 28 Feb 2012 15:32:29 +0530
Subject: [PATCH] microblaze: 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.holsgrove@xilinx.com>

  *  gcc/config/microblaze/microblaze.c: microblaze_has_swap = 0
     Add version check for v8.30.a to enable microblaze_has_swap
  *  gcc/config/microblaze/microblaze.h: Add TARGET_HAS_SWAP
  *  gcc/config/microblaze/microblaze.md: New bswapsi2 and bswaphi2
     instructions
  *  gcc/config/microblaze/microblaze.opt: New options -mxl-reorder
     and -mno-xl-reorder

Signed-off-by: Nagaraju Mekala <nmekala@xilinx.com>
Signed-off-by: David Holsgrove <david.holsgrove@xilinx.com>
---
 gcc/config/microblaze/microblaze.c   |   38 ++++++++++++++++++++++++++++++++++
 gcc/config/microblaze/microblaze.h   |    5 ++++
 gcc/config/microblaze/microblaze.md  |   14 ++++++++++++
 gcc/config/microblaze/microblaze.opt |    8 +++++++
 4 files changed, 65 insertions(+), 0 deletions(-)

diff --git a/gcc/config/microblaze/microblaze.c b/gcc/config/microblaze/microblaze.c
index fc0296e..8819655 100644
--- a/gcc/config/microblaze/microblaze.c
+++ b/gcc/config/microblaze/microblaze.c
@@ -146,6 +146,9 @@ int microblaze_no_unsafe_delay;
 /* Set to one if the targeted core has the CLZ insn.  */
 int microblaze_has_clz = 0;
 
+/* Set to one if the targeted core has the swapb and swaph insn.  */
+int microblaze_has_swap = 0;
+
 /* Which CPU pipeline do we use. We haven't really standardized on a CPU 
    version having only a particular type of pipeline. There can still be 
    options on the CPU to scale pipeline features up or down. :( 
@@ -1380,6 +1383,41 @@ microblaze_option_override (void)
         microblaze_has_clz = 0;
     }
 
+  /* TARGET_REORDER defaults to 0 in microblaze.opt,
+     -mxl-reorder sets TARGET_REORDER to 1,
+     -mno-xl-reorder sets TARGET_NO_REORDER to 1.
+     Swap instructions are not to be emitted if TARGET_NO_REORDER == 1
+     but should be enabled by default if mcpu >= 8.30a */
+  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. */
+        if (TARGET_REORDER)
+          warning (0,
+                 "-mxl-reorder can be used only with -mcpu=v8.30.a or greater");
+    }
+  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;
+          }
+    }
+
   if (TARGET_MULTIPLY_HIGH && TARGET_SOFT_MUL)
     error ("-mxl-multiply-high requires -mno-xl-soft-mul");
 
diff --git a/gcc/config/microblaze/microblaze.h b/gcc/config/microblaze/microblaze.h
index a188a2e..76ee811 100644
--- a/gcc/config/microblaze/microblaze.h
+++ b/gcc/config/microblaze/microblaze.h
@@ -43,6 +43,7 @@ extern int microblaze_dbx_regno[];
 
 extern int microblaze_no_unsafe_delay;
 extern int microblaze_has_clz;
+extern int microblaze_has_swap;
 extern enum pipeline_type microblaze_pipe;
 
 #define OBJECT_FORMAT_ELF
@@ -62,6 +63,9 @@ extern enum pipeline_type microblaze_pipe;
 /* Do we have CLZ?  */
 #define TARGET_HAS_CLZ      (TARGET_PATTERN_COMPARE && microblaze_has_clz)
 
+/* Do we have SWAPB and SWAPH?  */
+#define TARGET_HAS_SWAP     (microblaze_has_swap)
+
 /* The default is to support PIC.  */
 #define TARGET_SUPPORTS_PIC 1
 
@@ -78,6 +82,7 @@ extern enum pipeline_type microblaze_pipe;
 	"%{mno-xl-barrel-shift:%<mxl-barrel-shift}", 	\
 	"%{mno-xl-pattern-compare:%<mxl-pattern-compare}", \
 	"%{mxl-soft-div:%<mno-xl-soft-div}", 		\
+	"%{mxl-reorder:%<mno-xl-reorder}", 		\
 	"%{msoft-float:%<mhard-float}"
 
 /* Tell collect what flags to pass to nm.  */
diff --git a/gcc/config/microblaze/microblaze.md b/gcc/config/microblaze/microblaze.md
index 1b42003..2e636c0 100644
--- a/gcc/config/microblaze/microblaze.md
+++ b/gcc/config/microblaze/microblaze.md
@@ -353,6 +353,20 @@
 (automata_option "time")
 (automata_option "progress")
 
+(define_insn "bswapsi2"
+  [(set (match_operand:SI 0 "register_operand" "=r")
+        (bswap:SI (match_operand:SI 1 "register_operand" "r")))]
+  "TARGET_HAS_SWAP"
+  "swapb %0, %1"
+)
+
+(define_insn "bswaphi2"
+  [(set (match_operand:HI 0 "register_operand" "=r")
+        (bswap:HI (match_operand:HI 1 "register_operand" "r")))]
+  "TARGET_HAS_SWAP"
+  "swaph %0, %1"
+)
+
 ;;----------------------------------------------------------------
 ;; Microblaze delay slot description
 ;;----------------------------------------------------------------
diff --git a/gcc/config/microblaze/microblaze.opt b/gcc/config/microblaze/microblaze.opt
index fc7d0cd..b461296 100644
--- a/gcc/config/microblaze/microblaze.opt
+++ b/gcc/config/microblaze/microblaze.opt
@@ -67,6 +67,14 @@ mxl-soft-mul
 Target Mask(SOFT_MUL)
 Use the soft multiply emulation (default)
 
+mxl-reorder
+Target RejectNegative Mask(REORDER)
+Use reorder instructions (default)
+
+mno-xl-reorder
+Target RejectNegative Var(TARGET_NO_REORDER)
+Do not use reorder instructions
+
 mxl-soft-div
 Target Mask(SOFT_DIV)
 Use the software emulation for divides (default)
-- 
1.7.3.2

Re: [Patch, microblaze]: Add support for swap instructions and reorder option

[Michael Eager]

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.holsgrove@xilinx.com>

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

RE: [Patch, microblaze]: Add support for swap instructions and reorder option

[David Holsgrove]

[-- Attachment #1: Type: text/plain, Size: 5231 bytes --]

Hi Michael,

Thanks for the review, please find comments inline below.

> -----Original Message-----
> From: Michael Eager [mailto:eager@eagerm.com]
> Sent: Wednesday, 27 February 2013 3:50 am
> To: David Holsgrove
> Cc: gcc-patches@gcc.gnu.org; Michael Eager (eager@eagercon.com); John
> Williams; Edgar E. Iglesias (edgar.iglesias@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.holsgrove@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.holsgrove@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	 eager@eagercon.com
> 1960 Park Blvd., Palo Alto, CA 94306  650-325-8077




[-- Attachment #2: 0001-microblaze-add-support-for-swap-instructions-and-reo.patch --]
[-- Type: application/octet-stream, Size: 4233 bytes --]

From f4ade6571bec1ccaf1c20d0da007b99fb9dccc83 Mon Sep 17 00:00:00 2001
From: David Holsgrove <david.holsgrove@xilinx.com>
Date: Tue, 28 Feb 2012 15:32:29 +0530
Subject: [PATCH] microblaze: add support for swap instructions and reorder option

Changelog

2013-02-27  David Holsgrove <david.holsgrove@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

Signed-off-by: Nagaraju Mekala <nmekala@xilinx.com>
Signed-off-by: David Holsgrove <david.holsgrove@xilinx.com>
---
 gcc/config/microblaze/microblaze.c   |   21 +++++++++++++++++++++
 gcc/config/microblaze/microblaze.h   |    1 +
 gcc/config/microblaze/microblaze.md  |   14 ++++++++++++++
 gcc/config/microblaze/microblaze.opt |    4 ++++
 4 files changed, 40 insertions(+), 0 deletions(-)

diff --git a/gcc/config/microblaze/microblaze.c b/gcc/config/microblaze/microblaze.c
index fc0296e..bf24c65 100644
--- a/gcc/config/microblaze/microblaze.c
+++ b/gcc/config/microblaze/microblaze.c
@@ -1380,6 +1380,27 @@ microblaze_option_override (void)
         microblaze_has_clz = 0;
     }
 
+  /* 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;
+    }
+
   if (TARGET_MULTIPLY_HIGH && TARGET_SOFT_MUL)
     error ("-mxl-multiply-high requires -mno-xl-soft-mul");
 
diff --git a/gcc/config/microblaze/microblaze.h b/gcc/config/microblaze/microblaze.h
index a188a2e..23ed16e 100644
--- a/gcc/config/microblaze/microblaze.h
+++ b/gcc/config/microblaze/microblaze.h
@@ -78,6 +78,7 @@ extern enum pipeline_type microblaze_pipe;
 	"%{mno-xl-barrel-shift:%<mxl-barrel-shift}", 	\
 	"%{mno-xl-pattern-compare:%<mxl-pattern-compare}", \
 	"%{mxl-soft-div:%<mno-xl-soft-div}", 		\
+	"%{mxl-reorder:%<mno-xl-reorder}", 		\
 	"%{msoft-float:%<mhard-float}"
 
 /* Tell collect what flags to pass to nm.  */
diff --git a/gcc/config/microblaze/microblaze.md b/gcc/config/microblaze/microblaze.md
index 1b42003..1eac941 100644
--- a/gcc/config/microblaze/microblaze.md
+++ b/gcc/config/microblaze/microblaze.md
@@ -353,6 +353,20 @@
 (automata_option "time")
 (automata_option "progress")
 
+(define_insn "bswapsi2"
+  [(set (match_operand:SI 0 "register_operand" "=r")
+        (bswap:SI (match_operand:SI 1 "register_operand" "r")))]
+  "TARGET_REORDER"
+  "swapb %0, %1"
+)
+
+(define_insn "bswaphi2"
+  [(set (match_operand:HI 0 "register_operand" "=r")
+        (bswap:HI (match_operand:HI 1 "register_operand" "r")))]
+  "TARGET_REORDER"
+  "swaph %0, %1"
+)
+
 ;;----------------------------------------------------------------
 ;; Microblaze delay slot description
 ;;----------------------------------------------------------------
diff --git a/gcc/config/microblaze/microblaze.opt b/gcc/config/microblaze/microblaze.opt
index fc7d0cd..13be973 100644
--- a/gcc/config/microblaze/microblaze.opt
+++ b/gcc/config/microblaze/microblaze.opt
@@ -67,6 +67,10 @@ mxl-soft-mul
 Target Mask(SOFT_MUL)
 Use the soft multiply emulation (default)
 
+mxl-reorder
+Target Var(TARGET_REORDER) Init(2)
+Use reorder instructions (default)
+
 mxl-soft-div
 Target Mask(SOFT_DIV)
 Use the software emulation for divides (default)
-- 
1.7.0.4

Re: [Patch, microblaze]: Add support for swap instructions and reorder option

[Michael Eager]

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:eager@eagerm.com]
>> Sent: Wednesday, 27 February 2013 3:50 am
>> To: David Holsgrove
>> Cc: gcc-patches@gcc.gnu.org; Michael Eager (eager@eagercon.com); John
>> Williams; Edgar E. Iglesias (edgar.iglesias@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	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077

RE: [Patch, microblaze]: Add support for swap instructions and reorder option

[David Holsgrove]

[-- Attachment #1: Type: text/plain, Size: 3462 bytes --]



> -----Original Message-----
> From: Michael Eager [mailto:eager@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.iglesias@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	 eager@eagercon.com
> 1960 Park Blvd., Palo Alto, CA 94306  650-325-8077




[-- Attachment #2: 0001-microblaze-add-support-for-swap-instructions-and-reo.patch --]
[-- Type: application/octet-stream, Size: 4016 bytes --]

From 3613566384dc3de4e0853281aee85e6cba44153c Mon Sep 17 00:00:00 2001
From: David Holsgrove <david.holsgrove@xilix.com>
Date: Tue, 28 Feb 2012 15:32:29 +0530
Subject: [PATCH] microblaze: add support for swap instructions and reorder option

Changelog

2013-02-27  David Holsgrove <david.holsgrove@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

Signed-off-by: Nagaraju Mekala <nmekala@xilinx.com>
Signed-off-by: David Holsgrove <david.holsgrove@xilinx.com>
---
 gcc/config/microblaze/microblaze.c   |   15 +++++++++++++++
 gcc/config/microblaze/microblaze.h   |    1 +
 gcc/config/microblaze/microblaze.md  |   14 ++++++++++++++
 gcc/config/microblaze/microblaze.opt |    4 ++++
 4 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/gcc/config/microblaze/microblaze.c b/gcc/config/microblaze/microblaze.c
index f45d30c..8458253 100644
--- a/gcc/config/microblaze/microblaze.c
+++ b/gcc/config/microblaze/microblaze.c
@@ -1380,6 +1380,21 @@ microblaze_option_override (void)
         microblaze_has_clz = 0;
     }
 
+  /* TARGET_REORDER defaults to 2 if -mxl-reorder not specified.  */
+  ver = MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v8.30.a");
+  if (ver < 0)
+    {
+        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) && !TARGET_PATTERN_COMPARE)
+    {
+        if (TARGET_REORDER == 1)
+          warning (0, "-mxl-reorder requires -mxl-pattern-compare for -mcpu=v8.30.a");
+        TARGET_REORDER = 0;
+    }
+
   if (TARGET_MULTIPLY_HIGH && TARGET_SOFT_MUL)
     error ("-mxl-multiply-high requires -mno-xl-soft-mul");
 
diff --git a/gcc/config/microblaze/microblaze.h b/gcc/config/microblaze/microblaze.h
index a188a2e..23ed16e 100644
--- a/gcc/config/microblaze/microblaze.h
+++ b/gcc/config/microblaze/microblaze.h
@@ -78,6 +78,7 @@ extern enum pipeline_type microblaze_pipe;
 	"%{mno-xl-barrel-shift:%<mxl-barrel-shift}", 	\
 	"%{mno-xl-pattern-compare:%<mxl-pattern-compare}", \
 	"%{mxl-soft-div:%<mno-xl-soft-div}", 		\
+	"%{mxl-reorder:%<mno-xl-reorder}", 		\
 	"%{msoft-float:%<mhard-float}"
 
 /* Tell collect what flags to pass to nm.  */
diff --git a/gcc/config/microblaze/microblaze.md b/gcc/config/microblaze/microblaze.md
index 3391864..9f17733 100644
--- a/gcc/config/microblaze/microblaze.md
+++ b/gcc/config/microblaze/microblaze.md
@@ -353,6 +353,20 @@
 (automata_option "time")
 (automata_option "progress")
 
+(define_insn "bswapsi2"
+  [(set (match_operand:SI 0 "register_operand" "=r")
+        (bswap:SI (match_operand:SI 1 "register_operand" "r")))]
+  "TARGET_REORDER"
+  "swapb %0, %1"
+)
+
+(define_insn "bswaphi2"
+  [(set (match_operand:HI 0 "register_operand" "=r")
+        (bswap:HI (match_operand:HI 1 "register_operand" "r")))]
+  "TARGET_REORDER"
+  "swaph %0, %1"
+)
+
 ;;----------------------------------------------------------------
 ;; Microblaze delay slot description
 ;;----------------------------------------------------------------
diff --git a/gcc/config/microblaze/microblaze.opt b/gcc/config/microblaze/microblaze.opt
index fc7d0cd..a659166 100644
--- a/gcc/config/microblaze/microblaze.opt
+++ b/gcc/config/microblaze/microblaze.opt
@@ -67,6 +67,10 @@ mxl-soft-mul
 Target Mask(SOFT_MUL)
 Use the soft multiply emulation (default)
 
+mxl-reorder
+Target Var(TARGET_REORDER) Init(2)
+Use reorder instructions (swap and byte reversed load/store) (default)
+
 mxl-soft-div
 Target Mask(SOFT_DIV)
 Use the software emulation for divides (default)
-- 
1.7.0.4

Re: [Patch, microblaze]: Add support for swap instructions and reorder option

[Michael Eager]

On 02/27/2013 04:36 PM, David Holsgrove wrote:
>
>
>> -----Original Message-----
>> From: Michael Eager [mailto:eager@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.iglesias@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 Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077

RE: [Patch, microblaze]: Add support for swap instructions and reorder option

[David Holsgrove]

[-- Attachment #1: Type: text/plain, Size: 927 bytes --]

Hi Michal,

> -----Original Message-----
> From: Michael Eager [mailto:eager@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.iglesias@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	 eager@eagercon.com
> 1960 Park Blvd., Palo Alto, CA 94306  650-325-8077
> 




[-- Attachment #2: 0001-Patch-microblaze-Update-gcc-doc-invoke.texi-for-Micr.patch --]
[-- Type: application/octet-stream, Size: 1736 bytes --]

From 4fb108213dd0523709fc1b674d164b4532929e63 Mon Sep 17 00:00:00 2001
From: David Holsgrove <david.holsgrove@xilinx.com>
Date: Mon, 4 Mar 2013 13:15:47 +1000
Subject: [PATCH] [Patch, microblaze]: Update gcc/doc/invoke.texi for MicroBlaze options

gcc/Changelog

2013-03-04  David Holsgrove <david.holsgrove@xilinx.com>

 * gcc/doc/invoke.texi (MicroBlaze): Add new option -mxl-reorder
   and missed options -mbig-endian -mlittle-endian

Signed-off-by: David Holsgrove <david.holsgrove@xilinx.com>
---
 gcc/doc/invoke.texi |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 75dd9bd..a2a4066 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -733,7 +733,7 @@ Objective-C and Objective-C++ Dialects}.
 -mmemcpy -mxl-soft-mul -mxl-soft-div -mxl-barrel-shift @gol
 -mxl-pattern-compare -mxl-stack-check -mxl-gp-opt -mno-clearbss @gol
 -mxl-multiply-high -mxl-float-convert -mxl-float-sqrt @gol
--mxl-mode-@var{app-model}}
+-mbig-endian -mlittle-endian -mxl-reorder -mxl-mode-@var{app-model}}
 
 @emph{MIPS Options}
 @gccoptlist{-EL  -EB  -march=@var{arch}  -mtune=@var{arch} @gol
@@ -15820,6 +15820,18 @@ Use hardware floating-point conversion instructions.
 @opindex mxl-float-sqrt
 Use hardware floating-point square root instruction.
 
+@item -mbig-endian
+@opindex mbig-endian
+Generate code for a big-endian target.
+
+@item -mlittle-endian
+@opindex mlittle-endian
+Generate code for a little-endian target.
+
+@item -mxl-reorder
+@opindex mxl-reorder
+Use reorder instructions (swap and byte reversed load/store).
+
 @item -mxl-mode-@var{app-model}
 Select application model @var{app-model}.  Valid models are
 @table @samp
-- 
1.7.0.4

Re: [Patch, microblaze]: Add support for swap instructions and reorder option

[Michael Eager]

On 03/05/2013 06:54 AM, David Holsgrove wrote:
> Hi Michal,
>
>> -----Original Message-----
>> From: Michael Eager [mailto:eager@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.iglesias@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.holsgrove@xilinx.com>

	* doc/invoke.texi (MicroBlaze): Add -mbig-endian, -mlittle-endian,
	-mxl-reorder.


Please remember to submit a ChangeLog with patches.


-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077