frv: asm reloads for pseudos assigned to memory fail

frv: asm reloads for pseudos assigned to memory fail

[Alexandre Oliva]

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

I got big, ugly piece of C code the other day that passed to an asm
statement a variable of type int, that had been previously assigned
the value of a 64-bit variable.

Turned out the asm ended up with subreg for the 64-bit variable, that
hadn't changed, and the corresponding pseudo ended up assigned to a
stack slot.

Reload emitted the input reload, and then immediately attempted to
recognize the emitted instruction, which it does for all reloads for
asms.  Unfortunately, the insn was recognized as movsi_internal, but
it failed strict constraint checking because we had a pseudo, and the
insn doesn't accept memory operands.

Even if I relaxed the check to be non-strict (since the next round of
reloading would presumably fix things up, I thought), it failed
because the pseudo had become a mem(fp+offset), but the insn had
already been recognized as one that doesn't accept memory inputs, so
it just fell apart.

The work around I came up with was to enable the pattern to accept
memory inputs that fit without reloading, such that it doesn't end up
loading constants from memory (the reason why the memory loads were
split into a separate insn).  I'm hoping this won't prevent mems from
matching whose addresses require reloading (e.g., an fp offset that is
too big to fit the immediate offset of a load instruction).

Tested on i686-pc-linux-gnu-x-frv-elf, along with another patch
needed for target libraries to build
<URL:http://gcc.gnu.org/ml/gcc-patches/2004-05/msg01795.html>.

Ok to install both?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gcc-frv-reload-asm-input-memory-pseudo.patch --]
[-- Type: text/x-patch, Size: 3966 bytes --]

Index: gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	* config/frv/frv.md (movqi_internal, movhi_internal,
	movsi_internal): Add backup alternatives for memory inputs.

Index: gcc/config/frv/frv.md
===================================================================
RCS file: /cvs/uberbaum/gcc/config/frv/frv.md,v
retrieving revision 1.16
diff -u -p -r1.16 frv.md
--- gcc/config/frv/frv.md 7 Jul 2004 19:24:12 -0000 1.16
+++ gcc/config/frv/frv.md 16 Jul 2004 18:51:03 -0000
@@ -1312,12 +1312,12 @@
    (set_attr "type" "gload,fload")])
 
 (define_insn "*movqi_internal"
-  [(set (match_operand:QI 0 "move_destination_operand" "=d,d,m,m,?f,?f,?d,?m,f")
-	(match_operand:QI 1 "move_source_operand"       "L,d,d,O, d, f, f, f,GO"))]
+  [(set (match_operand:QI 0 "move_destination_operand" "=d,d,m,m,?f,?f,?d,?m,f,d,f")
+	(match_operand:QI 1 "move_source_operand"       "L,d,d,O, d, f, f, f,GO,!m,!m"))]
   "register_operand(operands[0], QImode) || reg_or_0_operand (operands[1], QImode)"
   "* return output_move_single (operands, insn);"
   [(set_attr "length" "4")
-   (set_attr "type" "int,int,gstore,gstore,movgf,fsconv,movfg,fstore,movgf")])
+   (set_attr "type" "int,int,gstore,gstore,movgf,fsconv,movfg,fstore,movgf,gload,fload")])
 
 (define_expand "movhi"
   [(set (match_operand:HI 0 "general_operand" "")
@@ -1341,12 +1341,12 @@
    (set_attr "type" "gload,fload")])
 
 (define_insn "*movhi_internal"
-  [(set (match_operand:HI 0 "move_destination_operand" "=d,d,d,m,m,?f,?f,?d,?m,f")
-	(match_operand:HI 1 "move_source_operand"       "L,n,d,d,O, d, f, f, f,GO"))]
+  [(set (match_operand:HI 0 "move_destination_operand" "=d,d,d,m,m,?f,?f,?d,?m,f,d,f")
+	(match_operand:HI 1 "move_source_operand"       "L,n,d,d,O, d, f, f, f,GO,!m,!m"))]
   "register_operand(operands[0], HImode) || reg_or_0_operand (operands[1], HImode)"
   "* return output_move_single (operands, insn);"
-  [(set_attr "length" "4,8,4,4,4,4,4,4,4,4")
-   (set_attr "type" "int,multi,int,gstore,gstore,movgf,fsconv,movfg,fstore,movgf")])
+  [(set_attr "length" "4,8,4,4,4,4,4,4,4,4,4,4")
+   (set_attr "type" "int,multi,int,gstore,gstore,movgf,fsconv,movfg,fstore,movgf,gload,fload")])
 
 ;; Split 2 word load of constants into sethi/setlo instructions
 (define_split
@@ -1402,6 +1402,14 @@
 ;; The resulting sequences for loading constants into FPRs are preferable
 ;; even when we're not generating PIC code.
 
+;; However, if we don't accept input from memory at all in the generic
+;; movsi pattern, reloads for asm instructions that reference pseudos
+;; that end up assigned to memory will fail to match, because we
+;; recognize them right after they're emitted, and we don't
+;; re-recognize them again after the substitution for memory.  So keep
+;; a memory constraint available, just make sure reload won't be
+;; tempted to use it.
+
 (define_insn "*movsi_load"
   [(set (match_operand:SI 0 "register_operand" "=d,f")
 	(match_operand:SI 1 "frv_load_operand" "m,m"))]
@@ -1436,12 +1444,12 @@
    (set_attr "length" "4")])
 
 (define_insn "*movsi_internal"
-  [(set (match_operand:SI 0 "move_destination_operand" "=d,d,d,m,m,z,d,d,f,f,m,?f,?z")
-	(match_operand:SI 1 "move_source_operand"      "L,n,d,d,O,d,z,f,d,f,f,GO,GO"))]
+  [(set (match_operand:SI 0 "move_destination_operand" "=d,d,d,m,m,z,d,d,f,f,m,?f,?z,d,f")
+	(match_operand:SI 1 "move_source_operand"      "L,n,d,d,O,d,z,f,d,f,f,GO,GO,!m,!m"))]
   "register_operand (operands[0], SImode) || reg_or_0_operand (operands[1], SImode)"
   "* return output_move_single (operands, insn);"
-  [(set_attr "length" "4,8,4,4,4,4,4,4,4,4,4,4,4")
-   (set_attr "type" "int,multi,int,gstore,gstore,spr,spr,movfg,movgf,fsconv,fstore,movgf,spr")])
+  [(set_attr "length" "4,8,4,4,4,4,4,4,4,4,4,4,4,4,4")
+   (set_attr "type" "int,multi,int,gstore,gstore,spr,spr,movfg,movgf,fsconv,fstore,movgf,spr,gload,fload")])
 
 ;; Split 2 word load of constants into sethi/setlo instructions
 (define_insn_and_split "*movsi_2word"

[-- Attachment #3: Type: text/plain, Size: 189 bytes --]



-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: frv: asm reloads for pseudos assigned to memory fail

[Richard Henderson]

On Sat, Jul 17, 2004 at 01:58:03AM -0300, Alexandre Oliva wrote:
> Even if I relaxed the check to be non-strict (since the next round of
> reloading would presumably fix things up, I thought), it failed
> because the pseudo had become a mem(fp+offset), but the insn had
> already been recognized as one that doesn't accept memory inputs, so
> it just fell apart.

Yes.  Reload *relies* on having a single move pattern.  FRV has
split its move patterns into at least two pieces, which is 
pretty much always going to fail.

Your patch merely papers over the problem.  Merge all the move
patterns back into one.


r~

Re: frv: asm reloads for pseudos assigned to memory fail

[Alexandre Oliva]

On Jul 28, 2004, Richard Henderson <rth@redhat.com> wrote:

> On Sat, Jul 17, 2004 at 01:58:03AM -0300, Alexandre Oliva wrote:
>> Even if I relaxed the check to be non-strict (since the next round of
>> reloading would presumably fix things up, I thought), it failed
>> because the pseudo had become a mem(fp+offset), but the insn had
>> already been recognized as one that doesn't accept memory inputs, so
>> it just fell apart.

> Yes.  Reload *relies* on having a single move pattern.  FRV has
> split its move patterns into at least two pieces, which is 
> pretty much always going to fail.

This was exactly the failure mode I ran into, and it's exactly what
this patch fixes.

We do want a separate pattern for loads for the reasons explained
above movsi_load.  What we don't want is to reject memory inputs in
the generic pattern, and this is exactly what the patch fixes, by
introducing equivalent patterns there.

> Your patch merely papers over the problem.

I don't think so.  If you think the arrangement after the patch could
still fail, please let me know how.

> Merge all the move patterns back into one.

I could, but I'd be reverting someone else's improvement.  I'd rather
not do it unless I'm shown there's something wrong with the it, other
than the bug I'm fixing in this patch.

-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: frv: asm reloads for pseudos assigned to memory fail

[Alexandre Oliva]

On Jul 28, 2004, Alexandre Oliva <aoliva@redhat.com> wrote:

> On Jul 28, 2004, Richard Henderson <rth@redhat.com> wrote:

>> Your patch merely papers over the problem.

> I don't think so.  If you think the arrangement after the patch could
> still fail, please let me know how.

>> Merge all the move patterns back into one.

> I could, but I'd be reverting someone else's improvement.  I'd rather
> not do it unless I'm shown there's something wrong with the it, other
> than the bug I'm fixing in this patch.

We had a long discussion on IRC about why FRV had to be different in
this regard.  The reason is that FRV FDPIC doesn't use a fixed PIC
register, and takes care of making sure we won't add new references to
the GOT value during reload.  These patterns are part of this.
Richard ended up approving the patch, so I'm checking it in.

-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: frv: asm reloads for pseudos assigned to memory fail

[Joern Rennecke]

> We had a long discussion on IRC about why FRV had to be different in
> this regard.  The reason is that FRV FDPIC doesn't use a fixed PIC
> register, and takes care of making sure we won't add new references to
> the GOT value during reload.  These patterns are part of this.
> Richard ended up approving the patch, so I'm checking it in.

But frv_cannot_force_const_mem already takes care of the PIC problem,
and you can avoid inadvertantly using a load in the non-PIC case by making
the load alternatives expensive enough.

Re: frv: asm reloads for pseudos assigned to memory fail

[Alexandre Oliva]

On Jul 29, 2004, Joern Rennecke <joern.rennecke@superh.com> wrote:

> and you can avoid inadvertantly using a load in the non-PIC case by making
> the load alternatives expensive enough.

Bzzzt!  Wrong.  :-)

Making the alternatives expensive enough will just make it more
difficult for them to be chosen.  It won't avoid it entirely, unless
you add so many `?'s that it becomes equivalent to `!' as far as
reload is concerned.

-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: frv: asm reloads for pseudos assigned to memory fail

[Joern Rennecke]

> > and you can avoid inadvertantly using a load in the non-PIC case by making
> > the load alternatives expensive enough.
> 
> Bzzzt!  Wrong.  :-)
> 
> Making the alternatives expensive enough will just make it more
> difficult for them to be chosen.  It won't avoid it entirely, unless
> you add so many `?'s that it becomes equivalent to `!' as far as
> reload is concerned.

As I said before, frv_cannot_force_const_mem already takes care of the
PIC problem, so the remaining issues are just performance.
That being said, there are some sub-optimal paths that reload could
theoretically take in the SH port, but I haven't ever seen them taken
once the right cost cues and logic were in place.

At any rate, having separate load patterns will probably cost you more
in performance than it can possibly save you by avoiding stray
force_const_mem, since reload inheritance can only work well when
you have your move patterns written properly.