| Message ID | 55F2F575.6080609@arm.com |
|---|---|
| State | New |
| Headers | show |
Hi Kyrill, > On 11/09/15 09:51, Rainer Orth wrote: >> Kyrill Tkachov <kyrylo.tkachov@arm.com> writes: >> >>> On 10/09/15 12:43, Rainer Orth wrote: >>>> Hi Kyrill, >>>> >>>>> Rainer, could you please check that this patch still fixes the SPARC >>>>> regressions? >>>> unfortunately, it breaks sparc-sun-solaris2.10 bootstrap: compiling >>>> stage2 libiberty/regex.c FAILs: >>>> >>>> >>> Thanks for providing the preprocessed file. >>> I've reproduced and fixed the ICE in this version of the patch. >>> The problem was that I was taking the mode of x before the check >>> of whether a and b are MEMs, after which we would change x to an >>> address_mode reg, >>> thus confusing emit_move_insn. >>> >>> The fix is to take the mode of x and perform the >>> can_conditionally_move_p check >>> after that transformation. >>> >>> Bootstrapped and tested on aarch64 and x86_64. >>> The preprocessed regex.i that Rainer provided now compiles successfully >>> for me >>> on a sparc-sun-solaris2.10 stage-1 cross-compiler. >>> >>> Rainer, thanks for your help so far, could you please try out this patch? >> While bootstrap succeeds again, the testsuite regression in >> gcc.c-torture/execute/20071216-1.c reoccured. > Right, so I dug into the RTL dumps and I think this is a separate issue > that's being exacerbated by my patch. > The code tries to if-convert a block which contains a compare instruction > i.e. sets the CC register. > Now, bb_valid_for_noce_process_p should have caught this, and in particular > insn_valid_noce_process_p > which should check that the instruction doesn't set the CC > register. However, on SPARC the > cc_in_cond returns NULL! This is due to the canonicalize_comparison > implementation that seems to > remove the CC register from the condition expression and returns something like: > (leu (reg/v:SI 109 [ b ]) > (const_int -4096 [0xfffffffffffff000]) > > Therefore the set_of (cc_in_cond (cond), insn) check doesn't get triggered > because cc_in_cond returns NULL. > Regardless of how the branch condition got canonicalized, I think we still > want to reject any insn in the block > that sets a condition code register, so this patch checks the destination > of every set in the block for a MODE_CC > expression and cancels if-conversion if that's the case. > > Oleg pointed me to the older PR 58517 affecting SH which seems similar and > I think my previous ifcvt patch would expose > this problem more. > > Anyway, with this patch the failing SPARC testcase > gcc.c-torture/execute/20071216-1.c generates the same assembly > as before r227368 and bootstrap and test on aarch64 and x86_64 passes ok for me. > > Rainer, could you try this patch on top of the previous patch? > (https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00689.html) > The two together should fix all of PR 67456, 67464, 67465 and 67481. sorry this took me so long: we've had a major switch failure and my sparc machines are way slow. Anyway, here's what I found: the two patches on top of each other do bootstrap just fine and the gcc.c-torture/execute/20071216-1.c regressions are gone. However, it introduces a new one: FAIL: gcc.dg/torture/stackalign/sibcall-1.c -O1 -fpic execution test It fails for both 32 and 64-bit. The testcase SEGVs: Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 1 (LWP 1)] 0x00010bb0 in ix86_split_ashr (mode=1) at /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/torture/stackalign/sibcall-1.c:20 20 : gen_x86_64_shrd) (0); (gdb) where #0 0x00010bb0 in ix86_split_ashr (mode=1) at /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/torture/stackalign/sibcall-1.c:20 #1 0x00010be4 in main () at /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/torture/stackalign/sibcall-1.c:27 1: x/i $pc => 0x10bb0 <ix86_split_ashr+40>: ld [ %g1 ], %g1 (gdb) p/x $g1 $1 = 0x317f8 truss reveals: Incurred fault #6, FLTBOUNDS %pc = 0x00010BB0 siginfo: SIGSEGV SEGV_MAPERR addr=0x000317F8 Received signal #11, SIGSEGV [default] siginfo: SIGSEGV SEGV_MAPERR addr=0x000317F8 with #define FLTBOUNDS 6 /* Memory bounds (invalid address) */ and indeed that address isn't mapped according to ro@apoc 58 > pmap 26536 26536: /var/gcc/regression/trunk/11-gcc/build/gcc/testsuite/gcc/sibcall-1.exe 00010000 8K r-x-- /var/gcc/regression/trunk/11-gcc/build/gcc/testsuite/gcc/sibcall-1.exe 00020000 8K rwx-- /var/gcc/regression/trunk/11-gcc/build/gcc/testsuite/gcc/sibcall-1.exe FEE60000 696K r-x-- /lib/libm.so.2 FEF1C000 16K rwx-- /lib/libm.so.2 FF180000 1464K r-x-- /lib/libc.so.1 FF2FE000 48K rwx-- /lib/libc.so.1 FF350000 24K rwx-- [ anon ] FF360000 8K rw--- [ anon ] FF370000 8K rw--- [ anon ] FF380000 8K rw--- [ anon ] FF390000 8K rw--- [ anon ] FF3A0000 248K r-x-- /lib/ld.so.1 FF3EE000 16K rwx-- /lib/ld.so.1 FFBFC000 16K rwx-- [ stack ] total 2576K Something is totally amiss here. Rainer
diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c index 9af3249..090a584 100644 --- a/gcc/ifcvt.c +++ b/gcc/ifcvt.c @@ -1838,6 +1838,19 @@ noce_try_cmove (struct noce_if_info *if_info) return FALSE; } +/* Return true if X contains a conditional code mode rtx. */ + +static bool +contains_ccmode_rtx_p (rtx x) +{ + subrtx_iterator::array_type array; + FOR_EACH_SUBRTX (iter, array, x, ALL) + if (GET_MODE_CLASS (GET_MODE (*iter)) == MODE_CC) + return true; + + return false; +} + /* Helper for bb_valid_for_noce_process_p. Validate that the rtx insn INSN is a single set that does not set the conditional register CC and is in general valid for @@ -1856,6 +1869,7 @@ insn_valid_noce_process_p (rtx_insn *insn, rtx cc) /* Currently support only simple single sets in test_bb. */ if (!sset || !noce_operand_ok (SET_DEST (sset)) + || contains_ccmode_rtx_p (SET_DEST (sset)) || !noce_operand_ok (SET_SRC (sset))) return false;