[Bug target/45233] FAIL: gcc.c-torture/compile/pr44707.c

[Bug target/45233] FAIL: gcc.c-torture/compile/pr44707.c

[iains at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45233

Iain Sandoe <iains at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2011.03.23 09:26:39
                 CC|                            |iains at gcc dot gnu.org,
                   |                            |mrs at gcc dot gnu.org
     Ever Confirmed|0                           |1

--- Comment #1 from Iain Sandoe <iains at gcc dot gnu.org> 2011-03-23 09:26:39 UTC ---
confirmed

for O1 :

Apparently, we have concluded that the indirect references are not required
when there is a 0 offset for the var.   When the offset is non-zero (v.b, v.c,
v.d), we correctly construct the indirect ref.  

for O0, all the references are correctly formed (and there is a corresponding
L_w$non_lazy_ptr).

---

    .text
    .align    2
    .globl _foo
_foo:
    stw r31,-4(r1)    ;,
    mflr r0    ;,
    bcl 20,31,L00000000001$pb    ;
L00000000001$pb:    ;
    mflr r31    ;,
    mtlr r0    ;,
    addis r2,r31,ha16(_v-L00000000001$pb)    ;,,
    la r10,lo16(_v-L00000000001$pb)(r2)    ;,,
    addis r2,r31,ha16(_w-L00000000001$pb)    ;,,
    la r11,lo16(_w-L00000000001$pb)(r2)    ;,,
    addis r2,r31,ha16(L_v$non_lazy_ptr-L00000000001$pb)    ;,,
    lwz r2,lo16(L_v$non_lazy_ptr-L00000000001$pb)(r2)    ;,,
    addis r8,r31,ha16(L_v$non_lazy_ptr-L00000000001$pb)    ;,,
    lwz r8,lo16(L_v$non_lazy_ptr-L00000000001$pb)(r8)    ;,,
    addis r9,r31,ha16(L_v$non_lazy_ptr-L00000000001$pb)    ;,,
    lwz r9,lo16(L_v$non_lazy_ptr-L00000000001$pb)(r9)    ;,,
; 12
"/Volumes/ScratchCS/gcc-live-trunk/gcc/testsuite/gcc.c-torture/compile/pr44707.c"
1
    /* 0(r10) 0(r11) 4(r2) 8(r8) 12(r9) */    ; v.a, w, v.b, v.c, v.d
; 0 "" 2
    lwz r31,-4(r1)    ;,
    blr    ;
    .non_lazy_symbol_pointer
L_v$non_lazy_ptr:
    .indirect_symbol _v
    .long    0
    .subsections_via_symbols

[Bug target/45233] FAIL: gcc.c-torture/compile/pr44707.c

[iains at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45233

--- Comment #2 from Iain Sandoe <iains at gcc dot gnu.org> 2011-11-06 12:36:23 UTC ---
Trying to simplify this, the following is enough to trigger the behavior:

extern int w;

void
foo (void)
{
  int e2 = w;
  __asm__ volatile ("/* %0 */" : : "ro" (e2));
}

==

(1) the code DTRT when the constraint is "r" or "o" ... but not when it is
"ro".

(2) essentially, up to combine, we have:


(insn 5 2 6 2 (set:SI (reg:SI 122)
        (plus:SI (reg:SI 31 r31)
            (high:SI (const:SI (unspec:SI [
                            (symbol_ref:SI ("&L_w$non_lazy_ptr") [flags 0x400] 
<var_decl 0x420a3a20 w>)
                        ] UNSPEC_MACHOPIC_OFFSET))))) ../tests/pr44707-x.c:7 96
{addsi3_high}
     (nil))

(insn 6 5 7 2 (set (reg/f:SI 121)
        (mem/u/c:SI (lo_sum:SI (reg:SI 122)
                (const:SI (unspec:SI [
                            (symbol_ref:SI ("&L_w$non_lazy_ptr") [flags 0x400] 
<var_decl 0x420a3a20 w>)
                        ] UNSPEC_MACHOPIC_OFFSET))) [0 S4 A8]))
../tests/pr44707-x.c:7 348 {movsi_low}
     (expr_list:REG_DEAD (reg:SI 122)
        (expr_list:REG_EQUAL (symbol_ref:SI ("w") [flags 0x240]  <var_decl
0x420a3a20 w>)
            (nil))))


(3) when "ro" is given, combine decides to delete insn 5 & 6  ...

(4) ira decides to re-insert the load of a pointer to w .. but it doesn't
(re-)insert the machopic indirect stuff...

(note 5 2 6 2 NOTE_INSN_DELETED)

(note 6 5 12 2 NOTE_INSN_DELETED)

(insn 12 6 13 2 (set (reg:SI 2 r2)
        (plus:SI (reg:SI 31 r31)
            (high:SI (const:SI (unspec:SI [
                            (symbol_ref:SI ("w") [flags 0x240]  <var_decl
0x420a3a20 w>)
                        ] UNSPEC_MACHOPIC_OFFSET))))) ../tests/pr44707-x.c:7 96
{addsi3_high}
     (nil))

(insn 13 12 7 2 (set (reg:SI 2 r2)
        (lo_sum:SI (reg:SI 2 r2)
            (const:SI (unspec:SI [
                        (symbol_ref:SI ("w") [flags 0x240]  <var_decl
0x420a3a20 w>)
                    ] UNSPEC_MACHOPIC_OFFSET)))) ../tests/pr44707-x.c:7 14
{macho_low_si}
     (nil))


which leads to the assembler fail...

====

So ... I don't know whether the problem lies in combine (incorrectly
eliminating the address load) -- or in ira (not re-introducing the indirect
reference).

Would welcome input from one of you gurus ... 
this is an irritating long-term fail (and, I guess, it might even have
implications for other targets...)

[Bug target/45233] FAIL: gcc.c-torture/compile/pr44707.c

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45233

--- Comment #3 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-11-06 12:46:43 UTC ---
Well, I guess the testcase is simply invalid for MachO, or the way MachO
does this UNSPEC stuff is broken (not properly checked during legitimization)
or not properly restoring the UNSPEC during reload (it's its duty, not IRAs).

[Bug target/45233] FAIL: gcc.c-torture/compile/pr44707.c

[iains at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45233

--- Comment #4 from Iain Sandoe <iains at gcc dot gnu.org> 2011-11-06 13:00:18 UTC ---
(In reply to comment #3)

Thanks..

> Well, I guess the testcase is simply invalid for MachO, 

well it works for -O0 (and for x86 darwin) ... so let's assume that MachO can
do it  ...

> or the way MachO
> does this UNSPEC stuff is broken (not properly checked during legitimization)

OK, well I'll look at those (although I don't see any dependancy on
optimization level in the legitimize code in rs6000.c or config/darwin.c).

> or not properly restoring the UNSPEC during reload (it's its duty, not IRAs).

I'll check this too.

[Bug target/45233] FAIL: gcc.c-torture/compile/pr44707.c

[iains at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45233

--- Comment #5 from Iain Sandoe <iains at gcc dot gnu.org> 2011-11-07 08:24:54 UTC ---
(In reply to comment #4)
> (In reply to comment #3)

FWIW gcc-4.2.1 (Apple local) fails and clang passes the test (although clang's
asm at version 2.9 is a bit long-winded c.f. GCC's ... but that's quite an old
version).

> > or not properly restoring the UNSPEC during reload (it's its duty, not IRAs).
> 
> I'll check this too.

Thanks Richi, this seems to be the problem - it appears that the reload
legitimizer does not consider that the item might be 'undefined' (in the
mach-o, local-to-the-file sense) - and, therefore, there was no mechanism to
recreate the necessary indirection.

so, this is a fix - which bootstraps and regtests:

I'd very much like an opinion as to whether it's the _right_ fix ... this is an
area with which I am not familiar and therefore could have missed some other
guard that's needed.

It's clearly a very infrequent (perhaps even non-existent) occurrence outside
the particular asm test-case (which makes me wonder still if combine came to
the right decision to remove the refs in the first place).

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c  (revision 181027)
+++ gcc/config/rs6000/rs6000.c  (working copy)
@@ -6185,7 +6185,21 @@ rs6000_legitimize_reload_address (rtx x, enum mach
 #if TARGET_MACHO
       if (flag_pic)
        {
-         rtx offset = machopic_gen_offset (x);
+         rtx offset;
+
+         /* Reload might present us with a case where there is a reference to
+            an undefined entity.  */
+         if (!machopic_symbol_defined_p (x) && !MACHO_DYNAMIC_NO_PIC_P)
+           {
+             offset = gen_rtx_SYMBOL_REF (Pmode, 
+                                          machopic_indirection_name (x, 
+                                                                     false));
+             SYMBOL_REF_DATA (offset) = SYMBOL_REF_DATA (x);
+             machopic_define_symbol (gen_const_mem (Pmode,offset));
+             x = offset;
+           }
+
+         offset = machopic_gen_offset (x);
          x = gen_rtx_LO_SUM (GET_MODE (x),
                gen_rtx_PLUS (Pmode, pic_offset_table_rtx,
                  gen_rtx_HIGH (Pmode, offset)), offset);

[Bug target/45233] FAIL: gcc.c-torture/compile/pr44707.c

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45233

--- Comment #6 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-11-07 09:28:17 UTC ---
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> 
> FWIW gcc-4.2.1 (Apple local) fails and clang passes the test (although clang's
> asm at version 2.9 is a bit long-winded c.f. GCC's ... but that's quite an old
> version).
> 
> > > or not properly restoring the UNSPEC during reload (it's its duty, not IRAs).
> > 
> > I'll check this too.
> 
> Thanks Richi, this seems to be the problem - it appears that the reload
> legitimizer does not consider that the item might be 'undefined' (in the
> mach-o, local-to-the-file sense) - and, therefore, there was no mechanism to
> recreate the necessary indirection.
> 
> so, this is a fix - which bootstraps and regtests:
> 
> I'd very much like an opinion as to whether it's the _right_ fix ... this is an
> area with which I am not familiar and therefore could have missed some other
> guard that's needed.
> 
> It's clearly a very infrequent (perhaps even non-existent) occurrence outside
> the particular asm test-case (which makes me wonder still if combine came to
> the right decision to remove the refs in the first place).

Combine also asks the target whether the result is valid, so yes.

> Index: gcc/config/rs6000/rs6000.c
> ===================================================================
> --- gcc/config/rs6000/rs6000.c  (revision 181027)
> +++ gcc/config/rs6000/rs6000.c  (working copy)
> @@ -6185,7 +6185,21 @@ rs6000_legitimize_reload_address (rtx x, enum mach
>  #if TARGET_MACHO
>        if (flag_pic)
>         {
> -         rtx offset = machopic_gen_offset (x);
> +         rtx offset;
> +
> +         /* Reload might present us with a case where there is a reference to
> +            an undefined entity.  */
> +         if (!machopic_symbol_defined_p (x) && !MACHO_DYNAMIC_NO_PIC_P)
> +           {
> +             offset = gen_rtx_SYMBOL_REF (Pmode, 
> +                                          machopic_indirection_name (x, 
> +                                                                     false));
> +             SYMBOL_REF_DATA (offset) = SYMBOL_REF_DATA (x);
> +             machopic_define_symbol (gen_const_mem (Pmode,offset));
> +             x = offset;
> +           }
> +
> +         offset = machopic_gen_offset (x);
>           x = gen_rtx_LO_SUM (GET_MODE (x),
>                 gen_rtx_PLUS (Pmode, pic_offset_table_rtx,
>                   gen_rtx_HIGH (Pmode, offset)), offset);

I suppose you should post this to gcc-patches@.

[Bug target/45233] FAIL: gcc.c-torture/compile/pr44707.c

[iains at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45233

--- Comment #7 from Iain Sandoe <iains at gcc dot gnu.org> 2011-11-07 13:37:16 UTC ---
still not right .. generates wrong code for the first two accesses (missing the
indirect load).

[Bug target/45233] FAIL: gcc.c-torture/compile/pr44707.c

[iains at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45233

--- Comment #8 from Iain Sandoe <iains at gcc dot gnu.org> 2011-11-09 10:03:31 UTC ---
well, I was trying to be too complicated - we should just avoid trying to do
the substitution unless we can see the var in the TU.  When this is done:

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c  (revision 181150)
+++ gcc/config/rs6000/rs6000.c  (working copy)
@@ -6169,6 +6169,7 @@ rs6000_legitimize_reload_address (rtx x, enum mach
 #if TARGET_MACHO
       && DEFAULT_ABI == ABI_DARWIN
       && (flag_pic || MACHO_DYNAMIC_NO_PIC_P)
+      && machopic_symbol_defined_p (x)
 #else
       && DEFAULT_ABI == ABI_V4
       && !flag_pic

.. the test-case generates beautiful (i.e. what one would have generated by
hand) asm.

reg-strapped on trunk (although there's a lot of noise on trunk right now
because of recent changes) 
- doing 4.6 branch now.

will post to patches if the 4.6 regstrap is also OK.

====

As an aside (non-Darwin comment - since powerpc-eabisim does exactly the same).

It's not obvious why the perfectly sensible RTL pre-combine is thrown away and
the process started over (I'm guessing it's because the asm insn has a zero
computed cost).  Effectively (to my untutored eye), we end up rebuilding what
was there before combine from the reload ...

[Bug target/45233] FAIL: gcc.c-torture/compile/pr44707.c

[mikestump at comcast dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45233

--- Comment #9 from Mike Stump <mikestump at comcast dot net> 2011-11-09 17:03:21 UTC ---
Ok.

Yeah, combine has a habit of removing a complex thing at one point and
rebuilding at another point, mainly to shorten the lifetime.  Mentally, I guess
I was expecting to see code motion to change lifetime.

[Bug target/45233] FAIL: gcc.c-torture/compile/pr44707.c

[dominiq at lps dot ens.fr]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45233

--- Comment #10 from Dominique d'Humieres <dominiq at lps dot ens.fr> 2011-11-11 22:41:36 UTC ---
The patch in comment #8 fixes this pr. I have only been able to regtest gcc and
gfortran without regression. Thanks.

[Bug target/45233] FAIL: gcc.c-torture/compile/pr44707.c

[iains at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45233

--- Comment #11 from Iain Sandoe <iains at gcc dot gnu.org> 2011-11-12 14:12:31 UTC ---
Author: iains
Date: Sat Nov 12 14:12:26 2011
New Revision: 181315

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=181315
Log:

gcc:

    PR target/45233
    * config/rs6000/rs6000.c (rs6000_legitimize_reload_address):
    Only expand a symbol ref. into an access when the entity is defined
    in the TU.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/rs6000/rs6000.c

[Bug target/45233] FAIL: gcc.c-torture/compile/pr44707.c

[iains at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45233

Iain Sandoe <iains at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

--- Comment #13 from Iain Sandoe <iains at gcc dot gnu.org> 2011-11-12 14:15:27 UTC ---
fixed.

[Bug target/45233] FAIL: gcc.c-torture/compile/pr44707.c

[iains at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45233

--- Comment #12 from Iain Sandoe <iains at gcc dot gnu.org> 2011-11-12 14:14:46 UTC ---
Author: iains
Date: Sat Nov 12 14:14:43 2011
New Revision: 181316

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=181316
Log:

gcc:

    PR target/45233
    * config/rs6000/rs6000.c (rs6000_legitimize_reload_address):
    Only expand a symbol ref. into an access when the entity is defined
    in the TU.


Modified:
    branches/gcc-4_6-branch/gcc/ChangeLog
    branches/gcc-4_6-branch/gcc/config/rs6000/rs6000.c