On 31/05/18 00:28, Jeff Law wrote:
It's not clear why having that subreg is causing incorrect code to be
generated, but the subreg is clearly wrong since it's a qisi pattern not
a hisi pattern. Based on that I've approved and installed the change.
There's some further info in PR85941, but I closed that since it didn't appear
combine was at fault.
If that subreg expression is in the insn pattern, then the expand RTL pass is
coerced into adding that additional expression so it is matched as
zero_extendqisi2 in the next pass:
(insn 11 10 12 2 (set (reg:SI 29)
(zero_extend:SI (subreg:HI (subreg/s/v:QI (reg:HI 23 [ _1 ]) 0) 0)))
By the time we get to the combine pass, the zero extend part looks like:
(zero_extend:SI (subreg:HI (reg:QI 28) 0)))
The expression to zero_extend then becomes R12:HI, losing the zero extension
from QImode. See below from the combine rtl dump:
insn_cost 4 for 8: r28:QI=R12:QI
REG_DEAD R12:QI
...
insn_cost 8 for 11: r29:SI=zero_extend(r28:QI#0)
REG_DEAD r28:QI
...
allowing combination of insns 8 and 11
original costs 4 + 8 = 12
replacement cost 8
deferring deletion of insn with uid = 8.
modifying insn i3 11: r29:SI=zero_extend(R12:HI)
REG_DEAD R12:QI
I guess combine decided it was valid to just directly use hard reg R12 in HImode
at this point.
I'm a big believer that any subreg appearing in an md is fishy and
should be reviewed and justified. I've found they're generally a bad idea.
Ok, there are quite a few uses of subreg expressions in msp430 insn patterns,
mostly when a PSImode operand is involved. I'll make a note to review these in
the future.
Thanks,
Jozef
