[PATCH, FT32] initial support

[PATCH, FT32] initial support

[James Bowman]

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

FT32 is a new high performance 32-bit RISC core developed by FTDI for embedded applications.

Support for FT32 has already been added to binutils. This patch adds FT32 support to gcc.
        
Please can someone review it, and if appropriate commit it, as I do not have write access to the tree.

The FSF have acknowledged receipt of FTDI's copyright assignment papers.

Thanks very much. ChangeLog entry:

2014-02-03  James Bowman  <james.bowman@ftdichip.com>

        * configure.ac: FT32 target added
        * libgcc/config.host: FT32 target added
        * gcc/config/ft32/: FT32 target added
        * libgcc/config/ft32/: FT32 target added
        * configure: Regenerated

--
James Bowman
FTDI Open Source Liaison

[-- Attachment #2: gcc-ft32.txt.gz --]
[-- Type: application/x-gzip, Size: 27412 bytes --]

Re: [PATCH, FT32] initial support

[Andrew Pinski]

On Mon, Feb 2, 2015 at 9:18 PM, James Bowman <james.bowman@ftdichip.com> wrote:
> FT32 is a new high performance 32-bit RISC core developed by FTDI for embedded applications.
>
> Support for FT32 has already been added to binutils. This patch adds FT32 support to gcc.
>
> Please can someone review it, and if appropriate commit it, as I do not have write access to the tree.
>
> The FSF have acknowledged receipt of FTDI's copyright assignment papers.

+(define_insn "umulsidi3"
+  [(set (match_operand:DI 0 "register_operand" "=r,r")
+    (mult:DI (zero_extend:DI (match_operand:SI 1 "register_operand" "r,r"))
+         (zero_extend:DI (match_operand:SI 2 "ft32_rimm_operand" "r,KA"))))
+  ]
+  ""
+  "mul.l  $cc,%1,%2\;muluh.l %h0,%1,%2\;move.l   %0,$cc")

Could you have a split of this instruction to allow better register
allocation to happen?
Also you are clobbering $cc but don't have a clobber for that register
in the pattern.

Likewise of:
+(define_insn "abssi2"
+  [(set (match_operand:SI 0 "register_operand" "=r")
+ (abs:SI (match_operand:SI 1 "register_operand" "r")))
+   (clobber (match_scratch:SI 2 "=&r"))]
+  ""
+  "ashr.l\t%2,%1,31\;xor.l\t%0,%1,%2\;sub.l\t%0,%0,%2")


You also have a few formatting issues dealing with if statements.
An example:
+  /* If this is a store, force the value into a register.  */
+  if ( 1 && (! (reload_in_progress || reload_completed)))
+  {
+    if (MEM_P (operands[0]))
+    {

it should be:
  if (!(reload_in_progress || reload_completed))
    {
      if (MEM_P (operands[0]))
        {
           ....
+  "
+{
+  /* If this is a store, force the value into a register.  */
+  if (MEM_P (operands[0]))
+    operands[1] = force_reg (SFmode, operands[1]);
+  if (CONST_DOUBLE_P(operands[1]))
+    operands[1] = force_const_mem(SFmode, operands[1]);
+}")
You don't need the quotes around the {} in end of the patterns any more.
An example:
+  "
+{
+  /* If this is a store, force the value into a register.  */
+  if (MEM_P (operands[0]))
+    operands[1] = force_reg (SFmode, operands[1]);
+  if (CONST_DOUBLE_P(operands[1]))
+    operands[1] = force_const_mem(SFmode, operands[1]);
+}")

You do some of the instructions have lengths but not all, the ones
which matter the most are the ones where the output is more than one
instruction.

You implement some of the sync_* patterns instead of the newer
atomic_* patterns.



Thanks,
Andrew Pinski


>
> Thanks very much. ChangeLog entry:
>
> 2014-02-03  James Bowman  <james.bowman@ftdichip.com>
>
>         * configure.ac: FT32 target added
>         * libgcc/config.host: FT32 target added
>         * gcc/config/ft32/: FT32 target added
>         * libgcc/config/ft32/: FT32 target added
>         * configure: Regenerated
>
> --
> James Bowman
> FTDI Open Source Liaison

Re: [PATCH, FT32] initial support

[Paolo Bonzini]

On 03/02/2015 07:05, Andrew Pinski wrote:
> Likewise of:
> +(define_insn "abssi2"
> +  [(set (match_operand:SI 0 "register_operand" "=r")
> + (abs:SI (match_operand:SI 1 "register_operand" "r")))
> +   (clobber (match_scratch:SI 2 "=&r"))]
> +  ""
> +  "ashr.l\t%2,%1,31\;xor.l\t%0,%1,%2\;sub.l\t%0,%0,%2")
> 

optabs.c's expand_abs_nojump already knows this trick:

/* If this machine has expensive jumps, we can do integer absolute
   value of X as (((signed) x >> (W-1)) ^ x) - ((signed) x >> (W-1)),
   where W is the width of MODE. */

So if you define BRANCH_COST to be 2 or more there should be no need for
this pattern at all.

Paolo

RE: [PATCH, FT32] initial support

[James Bowman]

> optabs.c's expand_abs_nojump already knows this trick:
>       
> /* If this machine has expensive jumps, we can do integer absolute
>    value of X as (((signed) x >> (W-1)) ^ x) - ((signed) x >> (W-1)),
>    where W is the width of MODE. */
> 
> So if you define BRANCH_COST to be 2 or more there should be no need for
> this pattern at all.
        
Yes, I just confirmed this. With no abssi2 pattern, this:
        
  int side;
        
  int foo(int x)
  {
    side = x >> 31;
    return abs(x);
  }

does indeed produce:

  foo: 
          ashr.l $r1,$r0,31
          sta.l  side,$r1
          xor.l  $r0,$r1,$r0
          sub.l  $r0,$r0,$r1
          return
          

Thanks.

--
James Bowman
FTDI Open Source Liaison

Re: [PATCH, FT32] initial support

[Joseph Myers]

This patch is missing pieces such as Texinfo documentation (in
invoke.texi for target-specific options, at least) and config-list.mk
update so automatic builders verify that this target builds OK.  See
"Back End" in sourcebuild.texi and make sure that you have everything
relevant.

It's a good idea to make sure any new port builds cleanly on both 32-bit 
and 64-bit hosts when configured --enable-werror-always and the compiler 
used to build it is the same version of GCC (this provides equivalent 
coverage for being free of compilation warnings as bootstrap does for 
native tools).

> Index: gcc/config/ft32/ft32.md
> ===================================================================
> --- gcc/config/ft32/ft32.md	(revision 0)
> +++ gcc/config/ft32/ft32.md	(revision 0)
> @@ -0,0 +1,965 @@
> +;; Machine description for FT32
> +;; Copyright (C) 2009 Free Software Foundation, Inc.

Copyright years should be <year>-2015.

> +        internal_error ("internal error: 'h' applied to non-register operand");

internal_error already prints "internal compiler error: "; don't
include "internal error: " in the error string.  Use %< and %> as
quotes rather than ''.

> +            internal_error ("internal error: bad alignment: %d", i);

Likewise.

> +        int bf = ft32_as_bitfield(INTVAL(x));

Missing spaces before '(' in function and macro calls; check for any
other instances.

> +      /* if (REGNO (operand) > FT32_R13) internal_error ("internal error: bad register: %d", REGNO (operand));  wtf */

Even commented-out calls (if really needed) should be cleaned up.

> +  if (65536 <= cfun->machine->size_for_adjusting_sp) {
> +    error("Stack frame must be smaller than 64K");

Start diagnostics with a lowercase letter.  Note a missing space again.

> +#if 0
> +/* fixed-bit.h does not define functions for TA and UTA because
> +   that part is wrapped in #if MIN_UNITS_PER_WORD > 4.
> +   This would lead to empty functions for TA and UTA.
> +   Thus, supply appropriate defines as if HAVE_[U]TA == 1.
> +   #define HAVE_[U]TA 1 won't work because avr-modes.def
> +   uses ADJUST_BYTESIZE(TA,8) and fixed-bit.h is not generic enough
> +   to arrange for such changes of the mode size.  */

Don't include large amounts of #if 0 or commented-out code in a new
port (*any* such code needs a good justification).

> +ft32-*-elf)
> +	# tmake_file="ft32/t-ft32 t-softfp-sfdf t-softfp-excl t-softfp"

Why commented-out?  soft-fp is to be preferred to fp-bit (except for
8-bit and 16-bit ports where space is very critical).  Also,
t-softfp-excl shouldn't be needed for new ports; it's only really
relevant when soft-fp is being built for hard-float multilibs for some
reason (t-hardfp is preferred then).

If the issue is that you want to exclude some soft-fp functions from
being built because you have architecture-specific versions of them,
it should be straightforward to add a new softfp_exclusions variable
tht t-softfp respects (in a separate patch, please).

> +	# tm_file="$tm_file ft32/ft32-lib.h"

Unless you're using this file, don't include it in the patch at all.


-- 
Joseph S. Myers
joseph@codesourcery.com

RE: [PATCH, FT32] initial support

[James Bowman]

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

(Thanks to everyone for the review. I have reworked the submission as suggested. This does build cleanly with "--enable-werror-always")

FT32 is a new high performance 32-bit RISC core developed by FTDI for embedded applications.

Support for FT32 has already been added to binutils. This patch adds FT32 support to gcc.
        
Please can someone review it, and if appropriate commit it, as I do not have write access to the tree.

The FSF have acknowledged receipt of FTDI's copyright assignment papers.

Thanks very much. ChangeLog entry:

2014-02-11  James Bowman  <james.bowman@ftdichip.com>

        * configure.ac: FT32 target added
        * libgcc/config.host: FT32 target added
        * gcc/config/ft32/: FT32 target added
        * libgcc/config/ft32/: FT32 target added
        * gcc/doc/install.texi, invoke.texi, md.texi: FT32 details added
        * gcc/doc/contrib.texi, MAINTAINERS: self added
        * contrib/config-list.mk: FT32 target added
        * configure: Regenerated

--
James Bowman
FTDI Open Source Liaison

[-- Attachment #2: gcc-ft32.txt.gz --]
[-- Type: application/x-gzip, Size: 27447 bytes --]

RE: [PATCH, FT32] initial support

[Joseph Myers]

On Wed, 11 Feb 2015, James Bowman wrote:

> +@table @gcctabopt
> +
> +@item -mspace
> +@opindex mspace
> +Enable code-size optimizations.
> +Some of these optimizations incur a minor performance penalty.

We already have -Os, so why is an architecture-specific option for this 
needed?

> +A 16-bit signed constant (-32768..32767)

Use @minus{} for a minus sign in Texinfo documentation, and @dots{} 
instead of literal ".." or "...".

-- 
Joseph S. Myers
joseph@codesourcery.com

RE: [PATCH, FT32] initial support

[James Bowman]

> > +@table @gcctabopt
> > +
> > +@item -mspace
> > +@opindex mspace
> > +Enable code-size optimizations.
> > +Some of these optimizations incur a minor performance penalty.
> 
> We already have -Os, so why is an architecture-specific option for this 
> needed?

Code compiled with -mspace is somewhat slower than code without.
So we typically build *all* code with -Os, with everything
non-critical also compiled -mspace.

Is this a legitimate option or should I just use -Os?

--
James Bowman
FTDI Open Source Liaison

RE: [PATCH, FT32] initial support

[Joseph Myers]

On Wed, 11 Feb 2015, James Bowman wrote:

> > > +@table @gcctabopt
> > > +
> > > +@item -mspace
> > > +@opindex mspace
> > > +Enable code-size optimizations.
> > > +Some of these optimizations incur a minor performance penalty.
> > 
> > We already have -Os, so why is an architecture-specific option for this 
> > needed?
> 
> Code compiled with -mspace is somewhat slower than code without.
> So we typically build *all* code with -Os, with everything
> non-critical also compiled -mspace.

The typical way of doing that would be to compile the critical code with 
-O2, everything else with -Os.  It's expected -Os may produce slower code 
than -O2.

-- 
Joseph S. Myers
joseph@codesourcery.com

RE: [PATCH, FT32] initial support

[James Bowman]

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

> From: Joseph Myers
> ...
> > +@table @gcctabopt
> > +
> > +@item -mspace
> > +@opindex mspace
> > +Enable code-size optimizations.
> > +Some of these optimizations incur a minor performance penalty.
> 
> We already have -Os, so why is an architecture-specific option for this
> needed?
> 
> > +A 16-bit signed constant (-32768..32767)
> 
> Use @minus{} for a minus sign in Texinfo documentation, and @dots{}
> instead of literal ".." or "...".

I have updated the target options. Space-saving is now enabled by
-Os. There is also a new option -msim to enable building for the
simulator (the simulator is pending submission to gdb-binutils).

I have fixed the Texinfo formatting.

------------------------------------------------------------

FT32 is a new high performance 32-bit RISC core developed by FTDI for embedded applications.

Support for FT32 has already been added to binutils. This patch adds FT32 support to gcc.
        
Please can someone review it, and if appropriate commit it, as I do not have write access to the tree.

The FSF have acknowledged receipt of FTDI's copyright assignment papers.

Thanks very much. ChangeLog entry:

2014-02-16  James Bowman  <james.bowman@ftdichip.com>

        * configure.ac: FT32 target added
        * libgcc/config.host: FT32 target added
        * gcc/config/ft32/: FT32 target added
        * libgcc/config/ft32/: FT32 target added
        * gcc/doc/install.texi, invoke.texi, md.texi: FT32 details added
        * gcc/doc/contrib.texi, MAINTAINERS: self added
        * contrib/config-list.mk: FT32 target added
        * configure: Regenerated

--
James Bowman
FTDI Open Source Liaison

[-- Attachment #2: gcc-ft32.txt.gz --]
[-- Type: application/x-gzip, Size: 28825 bytes --]

RE: [PATCH, FT32] initial support

[Joseph Myers]

On Mon, 16 Feb 2015, James Bowman wrote:

> I have updated the target options. Space-saving is now enabled by
> -Os. There is also a new option -msim to enable building for the
> simulator (the simulator is pending submission to gdb-binutils).

The documentation in this patch doesn't seem to have been updated for 
those changes.

-- 
Joseph S. Myers
joseph@codesourcery.com

RE: [PATCH, FT32] initial support

[James Bowman]

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

Aha yes. Revised attached. invoke.texi now has:

These options are defined specifically for the FT32 port.

@table @gcctabopt

@item -msim
@opindex msim
Specifies that the program will be run on the simulator.  This causes
an alternate runtime startup and library to be linked.
You must not use this option when generating programs that will run on
real hardware; you must provide your own runtime library for whatever
I/O functions are needed.

@end table

--
James Bowman
FTDI Open Source Liaison


________________________________________
From: Joseph Myers [joseph@codesourcery.com]
Sent: Tuesday, February 17, 2015 2:06 AM
To: James Bowman
Cc: gcc-patches@gcc.gnu.org
Subject: RE: [PATCH, FT32] initial support

On Mon, 16 Feb 2015, James Bowman wrote:

> I have updated the target options. Space-saving is now enabled by
> -Os. There is also a new option -msim to enable building for the
> simulator (the simulator is pending submission to gdb-binutils).

The documentation in this patch doesn't seem to have been updated for
those changes.

--
Joseph S. Myers
joseph@codesourcery.com

[-- Attachment #2: gcc-ft32.txt.gz --]
[-- Type: application/x-gzip, Size: 28857 bytes --]

RE: [PATCH, FT32] initial support

[James Bowman]

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

> On Mon, 16 Feb 2015, James Bowman wrote:
> 
> > I have updated the target options. Space-saving is now enabled by
> > -Os. There is also a new option -msim to enable building for the
> > simulator (the simulator is pending submission to gdb-binutils).
> 
> The documentation in this patch doesn't seem to have been updated for
> those changes.
> 

Ping.
Also, have attached updated patchset for the current gcc. Thanks.

--
James Bowman
FTDI Open Source Liaison

[-- Attachment #2: gcc-ft32.txt.gz --]
[-- Type: application/x-gzip, Size: 29290 bytes --]

RE: [PATCH, FT32] initial support

[James Bowman]

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

Second ping.
Also, have attached updated patchset for the current gcc. Thanks.

--
James Bowman
FTDI Open Source Liaison


________________________________________
From: Joseph Myers [joseph@codesourcery.com]
Sent: Tuesday, February 17, 2015 2:06 AM
To: James Bowman
Cc: gcc-patches@gcc.gnu.org
Subject: RE: [PATCH, FT32] initial support

On Mon, 16 Feb 2015, James Bowman wrote:

> I have updated the target options. Space-saving is now enabled by
> -Os. There is also a new option -msim to enable building for the
> simulator (the simulator is pending submission to gdb-binutils).

The documentation in this patch doesn't seem to have been updated for
those changes.

--
Joseph S. Myers
joseph@codesourcery.com

[-- Attachment #2: gcc-ft32.txt.gz --]
[-- Type: application/x-gzip, Size: 28737 bytes --]

Re: [PATCH, FT32] initial support

[Jeff Law]

On 03/19/2015 09:28 AM, James Bowman wrote:
> Second ping.
> Also, have attached updated patchset for the current gcc. Thanks.
Thanks.  I don't see anything particularly worrisome from a correctness 
standpoint.  You may need to make minor updates to your .h file to cope 
with some of Trevor's work around eliminating conditionally compiled 
code.  Obviously the sooner we wrap up any lingering details the better 
since Trevor's work might be a bit of a moving target.

Similarly the work around changing the types of toplevel rtx structures 
to rtx_insn * from rtx may hit your .c and .md file in minor ways.  That 
work is also ongoing so will likely be a moving target as well. 
However, from a quick scan of ft32.c, I don't see much code that would 
likely be affected by those changes.

I didn't see any matching constraints in the md file, so you shouldn't 
be affected by the additional checking recently added to genrecog.

The change to the MAINTAINERS file will happen after you're approved by 
the steering committee as a maintainer.   For a new port like this it'd 
be silly to do anything other than have you as the maintainer, but we'll 
run through the usual procedure.

I assume you've got a copyright assignment on file with the FSF for this 
work?

I'm going to assume all the assembly code is correct :-)


Some minor notes:

You seem to have defined TRULY_NOOP_TRUNCATION twice.

Are shift counts truncated on the ft32?  You might want to define 
SHIFT_COUNT_TRUNCATED

Are you using LRA or reload?  The former is definitely preferred and for 
a simple target like this, I'd expect the transition to be very easy.

ft32.c should be following the GNU style conventions.  It's particularly 
bad in where it's placing the open/close braces.  We do

if (condition)
   {
     code
   }
else
   {
     more code
   }

But I see a lot of

if (condition) {
   code
} else {
   more code
}

Similarly you tend to have code like

if (cond1 &&
     cond2 &&
     cond3)

We write it as

if (cond1
     && cond2
     && cond3)

These seem minor, but a consistent convention really makes it easier 
when someone else has to fixup something in your port.  You can use 
"indent" to fix this stuff up quickly.


Jeff

Re: [PATCH, FT32] initial support

[Jeff Law]

On 05/08/2015 06:04 PM, James Bowman wrote:
>
>> Are you using LRA or reload?  The former is definitely preferred and for
>> a simple target like this, I'd expect the transition to be very easy.
>
> I'm using reload. Attempting to naively switch on LRA resulted in
>    internal compiler error: Max. number of generated reload insns per insn is achieved (90)
> so I would be very interested in a pointer an LRA clues.D Thanks.
Vladimir Makarov would be the best contact point.  I'm a bit surprised 
you ran into this issue given the simplicity of the port.

I'd minimize the testcase, then enable debugging dumps, then look at the 
insn that's causing LRA to loop.  If Vlad helps, he's probably going to 
want that minimized testcase as well, so it's time well spent.

multidelta and other tools exist which can help with the minimization 
process.

jeff

RE: [PATCH, FT32] initial support

[James Bowman]

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

> On 05/08/2015 06:04 PM, James Bowman wrote:
> >
> >> Are you using LRA or reload?  The former is definitely preferred and for
> >> a simple target like this, I'd expect the transition to be very easy.
> >
> > I'm using reload. Attempting to naively switch on LRA resulted in
> >    internal compiler error: Max. number of generated reload insns per insn is achieved (90)
> > so I would be very interested in a pointer an LRA clues.D Thanks.
> Vladimir Makarov would be the best contact point.  I'm a bit surprised
> you ran into this issue given the simplicity of the port.
>
> I'd minimize the testcase, then enable debugging dumps, then look at the
> insn that's causing LRA to loop.  If Vlad helps, he's probably going to
> want that minimized testcase as well, so it's time well spent.
>

It seems that with whenever a function's frame is bigger than 512 bytes,
LRA loops.  Likely this causes a problem because there is no actual
instruction for subtracting constants more than 512.  However, there is a
"link" pattern that allows this. It is puzzling.

Do you think it would be easier to make the submission as is, then debug
the LRA issues from that point? If so, I have attached the current patch set.

Thanks.

--
James Bowman
FTDI Open Source Liaison

[-- Attachment #2: gcc-ft32.txt.gz --]
[-- Type: application/x-gzip, Size: 28945 bytes --]

Re: [PATCH, FT32] initial support

[Jeff Law]

On 05/12/2015 04:17 PM, James Bowman wrote:
>> On 05/08/2015 06:04 PM, James Bowman wrote:
>>>
>>>> Are you using LRA or reload?  The former is definitely preferred and for
>>>> a simple target like this, I'd expect the transition to be very easy.
>>>
>>> I'm using reload. Attempting to naively switch on LRA resulted in
>>>     internal compiler error: Max. number of generated reload insns per insn is achieved (90)
>>> so I would be very interested in a pointer an LRA clues.D Thanks.
>> Vladimir Makarov would be the best contact point.  I'm a bit surprised
>> you ran into this issue given the simplicity of the port.
>>
>> I'd minimize the testcase, then enable debugging dumps, then look at the
>> insn that's causing LRA to loop.  If Vlad helps, he's probably going to
>> want that minimized testcase as well, so it's time well spent.
>>
>
> It seems that with whenever a function's frame is bigger than 512 bytes,
> LRA loops.  Likely this causes a problem because there is no actual
> instruction for subtracting constants more than 512.  However, there is a
> "link" pattern that allows this. It is puzzling.
Is the subtraction part of the prologue/epilogue or some address 
calculation elsewhere in the code?

What do you do prior to reload for subtracting a large constant? 
Presumably you copy it into a register then do a reg-reg subtraction?

You might start by setting a breakpoint in lra_emit_add and see what 
kinds of insns its emits.  If you're getting in there, I'd hazard a 
guess that it's generating reload insns that themselves need reloads.



>
> Do you think it would be easier to make the submission as is, then debug
> the LRA issues from that point? If so, I have attached the current patch set.
Not sure yet :-)  We really want to get away from the reload path and 
one way is to stop accepting ports that use it.  We've done similar 
things in the past with other deprecated codepaths.

It really depends on the complexity of getting LRA working for the 
target and given what I saw when looking at the port, I don't believe it 
should be much work.


>
> Thanks.
>
> --
> James Bowman
> FTDI Open Source Liaison
>

Re: [PATCH, FT32] initial support

[Mike Stump]

On May 12, 2015, at 3:36 PM, Jeff Law <law@redhat.com> wrote:
> 
> It really depends on the complexity of getting LRA working for the target and given what I saw when looking at the port, I don't believe it should be much work.

LRA should default to on?  Only preexisting ports about ask for, and get non-LRA compilers?  :-)

Re: [PATCH, FT32] initial support

[Segher Boessenkool]

On Tue, May 12, 2015 at 10:17:01PM +0000, James Bowman wrote:
> It seems that with whenever a function's frame is bigger than 512 bytes,
> LRA loops.  Likely this causes a problem because there is no actual
> instruction for subtracting constants more than 512.  However, there is a
> "link" pattern that allows this. It is puzzling.

That "link" pattern does

	(minus (reg) (imm))

but that is not canonical RTL; it should be written

	(plus (reg) (-imm))


Compile with -da to get dump files, look at the .reload one (which is
for LRA if that is enabled), stare long and hard.  I recommend coffee.

> Do you think it would be easier to make the submission as is, then debug
> the LRA issues from that point? If so, I have attached the current patch set.

You should add a -mlra option so other people can easily enable it, too;
also handy later (when it defaults to on) when LRA blows up (you can
workaround with -mno-lra then).


Segher

RE: [PATCH, FT32] initial support

[James Bowman]

> On Tue, May 12, 2015 at 10:17:01PM +0000, James Bowman wrote:
> > It seems that with whenever a function's frame is bigger than 512 bytes,
> > LRA loops.  Likely this causes a problem because there is no actual
> > instruction for subtracting constants more than 512.  However, there is a
> > "link" pattern that allows this. It is puzzling.
>
> That "link" pattern does
>
>         (minus (reg) (imm))
>
> but that is not canonical RTL; it should be written
>
>         (plus (reg) (-imm))
>

OK, thanks. I have corrected this. Seeing the same failure with LRA.

> Compile with -da to get dump files, look at the .reload one (which is
> for LRA if that is enabled), stare long and hard.  I recommend coffee.

OK! Looking more at this, the LRA is not actually looping on the link, but on an address calculation
So this code:

  int runtest_bigframe()
  {
    unsigned int i;
    unsigned char buf[900];

    for (i = 0; i < 900; i++)
      buf[i] = (i % 237);

Causes this address calculation, attempting to compute the address of buf[]:

(insn 36 30 37 2 (set (reg:SI 96)
        (const_int -900 [0xfffffffffffffc7c])) /home/james/tmp/x.c:11 25 {*movsi}
     (expr_list:REG_EQUIV (const_int -900 [0xfffffffffffffc7c])
        (nil)))
(insn 37 36 39 2 (set (reg:SI 97)
        (plus:SI (reg/f:SI 0 $fp)
            (reg:SI 96))) /home/james/tmp/x.c:11 2 {addsi3}
     (expr_list:REG_DEAD (reg:SI 96)
        (expr_list:REG_EQUIV (plus:SI (reg/f:SI 0 $fp)
                (const_int -900 [0xfffffffffffffc7c]))
            (nil))))

And LRA loops on insn 37, repeatedly allocating a register for (reg:SI 97).

However, something like this:

  void foo (void)
  {
    unsigned char buf[900];
    bar(buf);
  }

Happily compiles, even though the address calculation is identical!

  foo: 
          link   $fp,924
          ldk.l  $r0,-900
          add.l  $r0,$fp,$r0
          call   bar
          unlink $r29
          return

Here is the relevant part of the dump, just before reload as above:

(insn 11 6 7 2 (set (reg:SI 43)
        (const_int -900 [0xfffffffffffffc7c])) /home/james/tmp/x.c:31 25 {*movsi}
     (expr_list:REG_EQUIV (const_int -900 [0xfffffffffffffc7c])
        (nil)))
(insn 7 11 8 2 (set (reg:SI 2 $r0)
        (plus:SI (reg/f:SI 0 $fp)
            (reg:SI 43))) /home/james/tmp/x.c:31 2 {addsi3}
     (expr_list:REG_DEAD (reg:SI 43)
        (nil)))

They look very similar. I am currently baffled. 

> > Do you think it would be easier to make the submission as is, then debug
> > the LRA issues from that point? If so, I have attached the current patch set.
>
> You should add a -mlra option so other people can easily enable it, too;
> also handy later (when it defaults to on) when LRA blows up (you can
> workaround with -mno-lra then).

Sounds good to me. Would that be an acceptable way to get the FT32 port 
into the tree? I am very happy to go with the general flow towards LRA,
but at this point perhaps it may be a little early?

--
James Bowman
FTDI Open Source Liaison

Re: [PATCH, FT32] initial support

[Segher Boessenkool]

On Thu, May 14, 2015 at 01:44:48AM +0000, James Bowman wrote:
> > Compile with -da to get dump files, look at the .reload one (which is
> > for LRA if that is enabled), stare long and hard.  I recommend coffee.
> 
> OK! Looking more at this, the LRA is not actually looping on the link, but on an address calculation

[snip]

> And LRA loops on insn 37, repeatedly allocating a register for (reg:SI 97).

Now your job is to figure out why it does that.  It prints out for
every innstruction what alternative it chose, and why it thinks
that is a good plan or not.  Since nothing works, it puts in some
reload insns; it also shows those.  But since in your case the
reloads do not in fact solve anything, it loops.

Since you have a frame pointer, was this a -O0 compile?

[snip, working case:]

> (insn 7 11 8 2 (set (reg:SI 2 $r0)
>         (plus:SI (reg/f:SI 0 $fp)
>             (reg:SI 43))) /home/james/tmp/x.c:31 2 {addsi3}
>      (expr_list:REG_DEAD (reg:SI 43)
>         (nil)))
> 
> They look very similar. I am currently baffled. 

This doesn't need reloading the dest of the set, since it already has
a hard register.

> > > Do you think it would be easier to make the submission as is, then debug
> > > the LRA issues from that point? If so, I have attached the current patch set.
> >
> > You should add a -mlra option so other people can easily enable it, too;
> > also handy later (when it defaults to on) when LRA blows up (you can
> > workaround with -mno-lra then).
> 
> Sounds good to me. Would that be an acceptable way to get the FT32 port 
> into the tree?

I don't make those decisions.  Having -mlra is of course not enough;
but it can help you.  And it's trivial to implement, so why are we
talking about this :-)

There are other things wrong with your port as well, so you'll take some
time to fix those up anyway, and maybe the LRA issue isn't so hard at all.
Having a -mlra option is nice in any case, as long as you haven't converted
over to LRA fully.

> I am very happy to go with the general flow towards LRA,
> but at this point perhaps it may be a little early?

I think LRA should work fine for you, I didn't see anything excitingly
different from other archs in your port (but I didn't look in detail).


Do you have test results?


Segher

Re: [PATCH, FT32] initial support

[Segher Boessenkool]

On Thu, May 14, 2015 at 01:44:48AM +0000, James Bowman wrote:
> > On Tue, May 12, 2015 at 10:17:01PM +0000, James Bowman wrote:
> > > It seems that with whenever a function's frame is bigger than 512 bytes,
> > > LRA loops.

I cannot reproduce this with your testcase.  Please post the *exact*
testcase (nothing snipped) and the command line you used?


Segher

Re: [PATCH, FT32] initial support

[Segher Boessenkool]

On Thu, May 14, 2015 at 07:54:02AM -0500, Segher Boessenkool wrote:
> I cannot reproduce this with your testcase.  Please post the *exact*
> testcase (nothing snipped) and the command line you used?

Making the array volatile and using optimisation helped.  Kaboom.

It wants to reload pretty much everything.  Looking at your code,
it seems you use the ancient interfaces instead of
TARGET_LEGITIMATE_ADDRESS_P, I don't think LRA works with that?


Segher

RE: [PATCH, FT32] initial support

[James Bowman]

> On Thu, May 14, 2015 at 07:54:02AM -0500, Segher Boessenkool wrote:
> > I cannot reproduce this with your testcase.  Please post the *exact*
> > testcase (nothing snipped) and the command line you used?
> 
> Making the array volatile and using optimisation helped.  Kaboom.
> 
> It wants to reload pretty much everything.  Looking at your code,
> it seems you use the ancient interfaces instead of
> TARGET_LEGITIMATE_ADDRESS_P, I don't think LRA works with that?

The FT32 target uses TARGET_ADDR_SPACE_LEGITIMATE_ADDRESS_P
(ft32.c line 856).
The FT32 itself is a Harvard architecture, and the ft32 port uses
address spaces to distinguish between program and data memory.

Of the ports that currently support LRA (arc, mips, rs6000, s380, sh) none
use address spaces.
Perhaps this port's use of address spaces is causing the problem?

--
James Bowman
FTDI Open Source Liaison

Re: [PATCH, FT32] initial support

[Segher Boessenkool]

On Thu, May 14, 2015 at 03:20:46PM +0000, James Bowman wrote:
> > It wants to reload pretty much everything.  Looking at your code,
> > it seems you use the ancient interfaces instead of
> > TARGET_LEGITIMATE_ADDRESS_P, I don't think LRA works with that?
> 
> The FT32 target uses TARGET_ADDR_SPACE_LEGITIMATE_ADDRESS_P
> (ft32.c line 856).

Ah I see.

> The FT32 itself is a Harvard architecture, and the ft32 port uses
> address spaces to distinguish between program and data memory.
> 
> Of the ports that currently support LRA (arc, mips, rs6000, s380, sh) none
> use address spaces.
> Perhaps this port's use of address spaces is causing the problem?

That could certainly cause problems.  I don't think it's likely
it causes all this though.

Adding some debug, it looks like LRA never calls the TARGET_ADDR_SPACE_LEGITIMATE_ADDRESS_P
hook?  Not during the spilling it does, anyway.  Here is what it
_does_ do:

	 Choosing alt 1 in insn 12:  (0) r  (1) r  (2) r {addsi3}
      Creating newreg=58 from oldreg=56, assigning class GENERAL_REGS to r58
   12: r58:SI=$fp:SI+r55:SI
      REG_DEAD r55:SI
      REG_EQUIV $fp:SI-0x384
    Inserting insn reload after:
   32: r56:SI=r58:SI

so it decides to use the "r" constraint everywhere in that addsi3 insn, thinks
it isn't all okay yet (and it isn't, r56 was allocated to mem (see the ira dump),
has no hard reg yet), and solves it by generating an extra move.  That should be
a move from the reg to mem, but it doesn't want to:

            0 Non input pseudo reload: reject++
            1 Non pseudo reload: reject++
          alt=0,overall=608,losers=1,rld_nregs=1
            alt=1: Bad operand -- refuse
            0 Non input pseudo reload: reject++
            alt=2: Bad operand -- refuse
            0 Non input pseudo reload: reject++
            alt=3: Bad operand -- refuse
            0 Non input pseudo reload: reject++
            alt=4: Bad operand -- refuse
            0 Non input pseudo reload: reject++
            alt=5: Bad operand -- refuse
            alt=6: Bad operand -- refuse
            0 Non input pseudo reload: reject++
            alt=7: Bad operand -- refuse
            0 Non input pseudo reload: reject++
            alt=8: Bad operand -- refuse
	 Choosing alt 0 in insn 32:  (0) =r  (1) r {*movsi}
      Creating newreg=59 from oldreg=56, assigning class GENERAL_REGS to r59
   32: r59:SI=r58:SI
    Inserting insn reload after:
   33: r56:SI=r59:SI

The alternative that allows move to mem is alt 1, but it thinks the operand
doesn't match (it is "B" or "W"), it picks alt 0, loop, everyone unhappy.

"B" should match it seems?

(Why does IRA think r56 should be in memory?  Yeah, good question.)


Segher

Re: [PATCH, FT32] initial support

[Jeff Law]

On 05/14/2015 11:36 AM, Segher Boessenkool wrote:
> The alternative that allows move to mem is alt 1, but it thinks the operand
> doesn't match (it is "B" or "W"), it picks alt 0, loop, everyone unhappy.
>
> "B" should match it seems?
>
> (Why does IRA think r56 should be in memory?  Yeah, good question.)
Independent of that, if a reg-reg move generated by LRA (which is really 
a mem->reg or reg->mem) blows up on this target for some reason, then 
that needs to be addressed independently of whether or not IRA might 
have made a bad choice for which pseudo to spill.

Jeff

Re: [PATCH, FT32] initial support

[Segher Boessenkool]

On Thu, May 14, 2015 at 12:31:40PM -0600, Jeff Law wrote:
> On 05/14/2015 11:36 AM, Segher Boessenkool wrote:
> >The alternative that allows move to mem is alt 1, but it thinks the operand
> >doesn't match (it is "B" or "W"), it picks alt 0, loop, everyone unhappy.
> >
> >"B" should match it seems?
> >
> >(Why does IRA think r56 should be in memory?  Yeah, good question.)
> Independent of that, if a reg-reg move generated by LRA (which is really 
> a mem->reg or reg->mem) blows up on this target for some reason, then 
> that needs to be addressed independently of whether or not IRA might 
> have made a bad choice for which pseudo to spill.

Yes.  It probably is because of the "volatile" I inserted for it
to fail at all, anyway.

Changing constraints ABWef to be define_memory_constraint (instead of
define_constraint) makes this testcase pass.  It also makes the build
pass (it failed before; 90 reloads in libgcc).

Pretty miserable code for those memory accesses, but hey, better
than an ICE ;-)


Segher

Re: [PATCH, FT32] initial support

[Jeff Law]

On 05/14/2015 01:24 PM, Segher Boessenkool wrote:
> On Thu, May 14, 2015 at 12:31:40PM -0600, Jeff Law wrote:
>> On 05/14/2015 11:36 AM, Segher Boessenkool wrote:
>>> The alternative that allows move to mem is alt 1, but it thinks the operand
>>> doesn't match (it is "B" or "W"), it picks alt 0, loop, everyone unhappy.
>>>
>>> "B" should match it seems?
>>>
>>> (Why does IRA think r56 should be in memory?  Yeah, good question.)
>> Independent of that, if a reg-reg move generated by LRA (which is really
>> a mem->reg or reg->mem) blows up on this target for some reason, then
>> that needs to be addressed independently of whether or not IRA might
>> have made a bad choice for which pseudo to spill.
>
> Yes.  It probably is because of the "volatile" I inserted for it
> to fail at all, anyway.
>
> Changing constraints ABWef to be define_memory_constraint (instead of
> define_constraint) makes this testcase pass.  It also makes the build
> pass (it failed before; 90 reloads in libgcc).
>
> Pretty miserable code for those memory accesses, but hey, better
> than an ICE ;-)
Makes sense if it wasn't defined as a memory constraint that LRA would 
be having trouble -- odds are reload would have barf'd at some point too.

The only difference is I'm actually still better at understanding what 
reload's doing from its dumps than LRA.  It's hard to throw away decades 
of retained knowledge earned the hard way.

James, can you go ahead and make that change to ABWef and enable LRA and 
commit your changes to the trunk?

Thanks,
jeff

RE: [PATCH, FT32] initial support

[James Bowman]

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

> On 05/14/2015 01:24 PM, Segher Boessenkool wrote:
> > On Thu, May 14, 2015 at 12:31:40PM -0600, Jeff Law wrote:
> >> On 05/14/2015 11:36 AM, Segher Boessenkool wrote:
> >>> The alternative that allows move to mem is alt 1, but it thinks the operand
> >>> doesn't match (it is "B" or "W"), it picks alt 0, loop, everyone unhappy.
> >>>
> >>> "B" should match it seems?
> >>>
> >>> (Why does IRA think r56 should be in memory?  Yeah, good question.)
> >> Independent of that, if a reg-reg move generated by LRA (which is really
> >> a mem->reg or reg->mem) blows up on this target for some reason, then
> >> that needs to be addressed independently of whether or not IRA might
> >> have made a bad choice for which pseudo to spill.
> >
> > Yes.  It probably is because of the "volatile" I inserted for it
> > to fail at all, anyway.
> >
> > Changing constraints ABWef to be define_memory_constraint (instead of
> > define_constraint) makes this testcase pass.  It also makes the build
> > pass (it failed before; 90 reloads in libgcc).
> >
> > Pretty miserable code for those memory accesses, but hey, better
> > than an ICE ;-)
> Makes sense if it wasn't defined as a memory constraint that LRA would
> be having trouble -- odds are reload would have barf'd at some point too.
> 
> The only difference is I'm actually still better at understanding what
> reload's doing from its dumps than LRA.  It's hard to throw away decades
> of retained knowledge earned the hard way.
> 
> James, can you go ahead and make that change to ABWef and enable LRA and
> commit your changes to the trunk?

OK, ABWef changes made, and fixed a couple of pattern holes that they exposed.
Added a new target option "-mlra" to enable LRA.  Updated invoking.texi.

Please can someone commit this, if appropriate, as I do not have write access to the tree.

Thanks very much. ChangeLog entry:

2015-05-14  James Bowman  <james.bowman@ftdichip.com>

        * configure.ac: FT32 target added
        * libgcc/config.host: FT32 target added
        * gcc/config/ft32/: FT32 target added
        * libgcc/config/ft32/: FT32 target added
        * gcc/doc/install.texi, invoke.texi, md.texi: FT32 details added
        * gcc/doc/contrib.texi: self added
        * contrib/config-list.mk: FT32 target added
        * configure: Regenerated

--
James Bowman
FTDI Open Source Liaison

[-- Attachment #2: gcc-ft32.txt.gz --]
[-- Type: application/x-gzip, Size: 29337 bytes --]

Re: [PATCH, FT32] initial support

[Jeff Law]

On 05/14/2015 06:43 PM, James Bowman wrote:
>> On 05/14/2015 01:24 PM, Segher Boessenkool wrote:
>>> On Thu, May 14, 2015 at 12:31:40PM -0600, Jeff Law wrote:
>>>> On 05/14/2015 11:36 AM, Segher Boessenkool wrote:
>>>>> The alternative that allows move to mem is alt 1, but it thinks the operand
>>>>> doesn't match (it is "B" or "W"), it picks alt 0, loop, everyone unhappy.
>>>>>
>>>>> "B" should match it seems?
>>>>>
>>>>> (Why does IRA think r56 should be in memory?  Yeah, good question.)
>>>> Independent of that, if a reg-reg move generated by LRA (which is really
>>>> a mem->reg or reg->mem) blows up on this target for some reason, then
>>>> that needs to be addressed independently of whether or not IRA might
>>>> have made a bad choice for which pseudo to spill.
>>>
>>> Yes.  It probably is because of the "volatile" I inserted for it
>>> to fail at all, anyway.
>>>
>>> Changing constraints ABWef to be define_memory_constraint (instead of
>>> define_constraint) makes this testcase pass.  It also makes the build
>>> pass (it failed before; 90 reloads in libgcc).
>>>
>>> Pretty miserable code for those memory accesses, but hey, better
>>> than an ICE ;-)
>> Makes sense if it wasn't defined as a memory constraint that LRA would
>> be having trouble -- odds are reload would have barf'd at some point too.
>>
>> The only difference is I'm actually still better at understanding what
>> reload's doing from its dumps than LRA.  It's hard to throw away decades
>> of retained knowledge earned the hard way.
>>
>> James, can you go ahead and make that change to ABWef and enable LRA and
>> commit your changes to the trunk?
>
> OK, ABWef changes made, and fixed a couple of pattern holes that they exposed.
> Added a new target option "-mlra" to enable LRA.  Updated invoking.texi.
>
> Please can someone commit this, if appropriate, as I do not have write access to the tree.
>
> Thanks very much. ChangeLog entry:
>
> 2015-05-14  James Bowman  <james.bowman@ftdichip.com>
>
>          * configure.ac: FT32 target added
>          * libgcc/config.host: FT32 target added
>          * gcc/config/ft32/: FT32 target added
>          * libgcc/config/ft32/: FT32 target added
>          * gcc/doc/install.texi, invoke.texi, md.texi: FT32 details added
>          * gcc/doc/contrib.texi: self added
>          * contrib/config-list.mk: FT32 target added
>          * configure: Regenerated
Well, the plan will be for you to maintain the port.  So you should go 
ahead and get write access so that you can commit changes to the port.


See authenticated access on this page

https://gcc.gnu.org/svnwrite.html

List me as your sponsor

Thanks for your patience,
Jeff

Re: [PATCH, FT32] initial support

[Eric Botcazou]

> Thanks very much. ChangeLog entry:
> 
> 2015-05-14  James Bowman  <james.bowman@ftdichip.com>
> 
>         * configure.ac: FT32 target added
>         * libgcc/config.host: FT32 target added
>         * gcc/config/ft32/: FT32 target added
>         * libgcc/config/ft32/: FT32 target added
>         * gcc/doc/install.texi, invoke.texi, md.texi: FT32 details added
>         * gcc/doc/contrib.texi: self added
>         * contrib/config-list.mk: FT32 target added
>         * configure: Regenerated

That's wrong, you cannot add just a single entry to the toplevel ChangeLog, 
you need to add entries to the ChangeLog of every subdirectory instead: gcc, 
libgcc, contrib, etc.  Please fix.

-- 
Eric Botcazou

Re: [PATCH, FT32] initial support

[Jeff Law]

On 05/28/2015 03:50 PM, Eric Botcazou wrote:
>> Thanks very much. ChangeLog entry:
>>
>> 2015-05-14  James Bowman  <james.bowman@ftdichip.com>
>>
>>          * configure.ac: FT32 target added
>>          * libgcc/config.host: FT32 target added
>>          * gcc/config/ft32/: FT32 target added
>>          * libgcc/config/ft32/: FT32 target added
>>          * gcc/doc/install.texi, invoke.texi, md.texi: FT32 details added
>>          * gcc/doc/contrib.texi: self added
>>          * contrib/config-list.mk: FT32 target added
>>          * configure: Regenerated
>
> That's wrong, you cannot add just a single entry to the toplevel ChangeLog,
> you need to add entries to the ChangeLog of every subdirectory instead: gcc,
> libgcc, contrib, etc.  Please fix.
Done.
jeff

Re: [PATCH, FT32] initial support

[Jeff Law]

On 05/14/2015 09:20 AM, James Bowman wrote:
>
>> On Thu, May 14, 2015 at 07:54:02AM -0500, Segher Boessenkool wrote:
>>> I cannot reproduce this with your testcase.  Please post the *exact*
>>> testcase (nothing snipped) and the command line you used?
>>
>> Making the array volatile and using optimisation helped.  Kaboom.
>>
>> It wants to reload pretty much everything.  Looking at your code,
>> it seems you use the ancient interfaces instead of
>> TARGET_LEGITIMATE_ADDRESS_P, I don't think LRA works with that?
>
> The FT32 target uses TARGET_ADDR_SPACE_LEGITIMATE_ADDRESS_P
> (ft32.c line 856).
> The FT32 itself is a Harvard architecture, and the ft32 port uses
> address spaces to distinguish between program and data memory.
>
> Of the ports that currently support LRA (arc, mips, rs6000, s380, sh) none
> use address spaces.
> Perhaps this port's use of address spaces is causing the problem?
I don't think so, it feels like something else to me.  But I do have a 
larger concern that if LRA hasn't been used much, if at all, on targets 
that have multiple address spaces, then we may have other lurking issues.

Anyway:

In .ira we have:

(insn 11 7 12 2 (set (reg:SI 51)
         (const_int -900 [0xfffffffffffffc7c])) j.c:7 25 {*movsi}
      (expr_list:REG_EQUIV (const_int -900 [0xfffffffffffffc7c])
         (nil)))
(insn 12 11 18 2 (set (reg/f:SI 50)
         (plus:SI (reg/f:SI 0 $fp)
             (reg:SI 51))) j.c:7 2 {addsi3}
      (expr_list:REG_DEAD (reg:SI 51)
         (expr_list:REG_EQUIV (plus:SI (reg/f:SI 0 $fp)
                 (const_int -900 [0xfffffffffffffc7c]))
             (nil))))

Where r51 will get a hard reg, but r50 will not.  This is confirmed by 
looking at the coloring dump in .ira:

  Loop 0 (parent -1, header bb2, depth 0)
     bbs: 4 3 2
     all: 0r45 1r54 2r50 3r51 4r48 5r44 10r52 11r43
     modified regnos: 43 44 45 48 50 51 52 54
     border:
     Pressure: GENERAL_REGS=6
     Hard reg set forest:
       0:( 2-29)@0
         1:( 2-6 8-29)@87680
       Spill a2(r50,l0)

So r50 gets spilled to memory.  That's going to cause insn 12 to have an 
output reload because it can't store into memory directly.

         Choosing alt 1 in insn 12:  (0) r  (1) r  (2) r {addsi3}
       Creating newreg=55 from oldreg=50, assigning class GENERAL_REGS 
to r55
    12: r55:SI=$fp:SI+r51:SI
       REG_DEAD r51:SI
       REG_EQUIV $fp:SI-0x384
     Inserting insn reload after:
    32: r50:SI=r55:SI


So we're going to use r55 to hold the result of the arithmetic, then 
shove it into the stack slot allocated for r50.  That all seems normal.

But for some reason things blow up after that, it's almost as if LRA 
doesn't like the trival reg-reg copy emitted to handle the output reload.

At this point I'd recommend committing the port as-is and attacking LRA 
as a follow-up.

jeff