[Bug target/45233] FAIL: gcc.c-torture/compile/pr44707.c
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45233 --- Comment #12 from Iain Sandoe 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
[Bug target/45233] FAIL: gcc.c-torture/compile/pr44707.c
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45233 Iain Sandoe changed: What|Removed |Added Status|NEW |RESOLVED Resolution||FIXED --- Comment #13 from Iain Sandoe 2011-11-12 14:15:27 UTC --- fixed.
[Bug target/45233] FAIL: gcc.c-torture/compile/pr44707.c
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45233 --- Comment #11 from Iain Sandoe 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
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45233 --- Comment #10 from Dominique d'Humieres 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
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45233 --- Comment #9 from Mike Stump 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
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45233 --- Comment #8 from Iain Sandoe 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
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45233 --- Comment #7 from Iain Sandoe 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
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45233
--- Comment #6 from Richard Guenther 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
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45233
--- Comment #5 from Iain Sandoe 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
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45233 --- Comment #4 from Iain Sandoe 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
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45233 --- Comment #3 from Richard Guenther 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
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45233
--- Comment #2 from Iain Sandoe 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]
)
] 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]
)
] 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] )
(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] )
] 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] )
] 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
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45233 Iain Sandoe 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 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 .align2 .globl _foo _foo: stw r31,-4(r1);, mflr r0;, bcl 20,31,L001$pb; L001$pb:; mflr r31;, mtlr r0;, addis r2,r31,ha16(_v-L001$pb);,, la r10,lo16(_v-L001$pb)(r2);,, addis r2,r31,ha16(_w-L001$pb);,, la r11,lo16(_w-L001$pb)(r2);,, addis r2,r31,ha16(L_v$non_lazy_ptr-L001$pb);,, lwz r2,lo16(L_v$non_lazy_ptr-L001$pb)(r2);,, addis r8,r31,ha16(L_v$non_lazy_ptr-L001$pb);,, lwz r8,lo16(L_v$non_lazy_ptr-L001$pb)(r8);,, addis r9,r31,ha16(L_v$non_lazy_ptr-L001$pb);,, lwz r9,lo16(L_v$non_lazy_ptr-L001$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 .long0 .subsections_via_symbols
