On Fri, 5 Feb 2016, Bernd Schmidt wrote:
> On 02/05/2016 01:10 PM, Richard Biener wrote:
> > It fails
> >
> > FAIL: gcc.target/i386/addr-sel-1.c scan-assembler b\\\\+1
> >
> > on i?86 (or x86_64 -m32) though, generating
> >
> > f:
> > .LFB0:
> > .cfi_startproc
> > movl 4(%esp), %eax
> > leal 1(%eax), %edx
> > movsbl a+1(%eax), %eax
> > movsbl b(%edx), %edx
> > addl %edx, %eax
> > ret
>
> Well, it looks like the first movsbl load clobbers the potentially better base
> register, so trivial propagation doesn't work.
>
> It might be another case where allowing 2->2 in combine would help. Or
> enabling -frename-registers and rerunning reload_combine afterwards.
Before postreload we have
(insn 6 21 8 2 (parallel [
(set (reg:SI 1 dx [orig:87 _2 ] [87])
(plus:SI (reg:SI 0 ax [99])
(const_int 1 [0x1])))
(clobber (reg:CC 17 flags))
])
(insn 8 6 10 2 (set (reg:SI 0 ax [96])
(sign_extend:SI (mem/j:QI (plus:SI (reg:SI 1 dx [orig:87 _2 ]
[87])
(symbol_ref:SI ("a") [flags 0x2] <var_decl
0x7faa46f94cf0 a>)) [0 a S1 A8])))
(insn 10 8 11 2 (set (reg:SI 1 dx [98])
(sign_extend:SI (mem/j:QI (plus:SI (reg:SI 1 dx [orig:87 _2 ]
[87])
(symbol_ref:SI ("b") [flags 0x2] <var_decl
0x7faa46f94d80 b>)) [0 b S1 A8])))
so indeed the issue is not dx dieing in insn 10 but ax dieing in insn 8...
Maybe LRA can prefer to not do that if enough free registers are
available (that is, never re-use a register)?
With the above observation it seems less likely we can fix this
regression. Should I continue to find a less invasive fix like
by computing 'commutative' as it were before Andreas patch as well
and only if they disagree (thus Andreas patch introduced extra
swapping in recog_data) swap back?
The variant below fixes the testcase but I have to check if it
also fixes the 416.gamess regression (I would guess so).
The patch is of course quite arbitrary, basically reverting the
operand-swapping part of Andreas patch.
Thanks,
Richard.
Index: gcc/ira.c
===================================================================
--- gcc/ira.c (revision 233172)
+++ gcc/ira.c (working copy)
@@ -1774,7 +1774,7 @@ ira_setup_alts (rtx_insn *insn, HARD_REG
int nop, nalt;
bool curr_swapped;
const char *p;
- int commutative = -1;
+ int commutative = -1, alt_commutative = -1;
extract_insn (insn);
alternative_mask preferred = get_preferred_alternatives (insn);
@@ -1838,6 +1838,8 @@ ira_setup_alts (rtx_insn *insn, HARD_REG
case '%':
/* The commutative modifier is handled above. */
+ if (alt_commutative < 0)
+ alt_commutative = nop;
break;
case '0': case '1': case '2': case '3': case '4':
@@ -1889,10 +1891,13 @@ ira_setup_alts (rtx_insn *insn, HARD_REG
}
if (commutative < 0)
break;
+ /* Swap forth and back to avoid changing recog_data. */
+ if (! curr_swapped
+ || alt_commutative < 0)
+ std::swap (recog_data.operand[commutative],
+ recog_data.operand[commutative + 1]);
if (curr_swapped)
break;
- std::swap (recog_data.operand[commutative],
- recog_data.operand[commutative + 1]);
}
}
