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

[Michael Eager <eager at eagercon dot com>]

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