Re: [patch, avr] Take 2: Fix PR64331: insn output and insn length computation rely on REG_DEAD notes.

[Georg-Johann Lay <avr at gjlay dot de>]

Am 03/02/2015 um 05:12 PM schrieb Steven Bosscher:
> On Sat, Feb 28, 2015 at 5:38 PM, Georg-Johann Lay wrote:
> > Am 02/26/2015 um 11:45 PM schrieb Steven Bosscher:
> > > On Thu, Feb 26, 2015 at 8:35 PM, Georg-Johann Lay wrote:
> > > > Take #2 introduces a new, avr-specific rtl pass whose sole purpose is to
> > > > rectify notes.  The pass is scheduled right before cfg does down (right
> > > > before .*free_cfg) so that cfg and hence df machinery is available.
> > > >
> > > > Regression tests look fine and for the test case the patches produce
> > > > correct
> > > > code and correct insn length.
> > >
> > >
> > > Sorry for party-pooping, but it seems to me that the real bug is that
> > > the target is lying to reload.
> > >
> > > IIUC the AVR port selects the insn alternative after register
> > > allocation (after reload). It bases its selection on REG_DEAD notes.
> > > In PR64331 an alternative is used that clobbers r20 that has a
> > > REG_DEAD note, but r20 is not actually dead because hardreg-cprop has
> > > propagated it forward without adjusting the note.
> >
> >
> > It's not actually about constraint alternatives.
> >
> > Let me give an example: Testing HI for 0. The usual sequence would be
> >
> >    cc0 = reg.low == 0
> >    cc0 = cc0 && reg.high == 0
> >
> > which costs 2 instructions.  If reg is unused after, then ORing can be used
> > and cc0 will be set as a side effect.  This costs 1 insn:
> >
> >    cc0 = (reg.low |= reg.high)
> >
> > Using alternatives would double their number, i.e. 14 instead of 7 for
> > *cmphi.
> >
> > These constraint alternatives would have to express
> >
> > 1) reg-alloc, please, use alternative #1 (with clobber of reg)
> >     if the register is unused after
> >
> > 2) reg-alloc, please, use alternative #2 (not clobbering reg)
> >     if the register is used after
> >
> > If we assume for a moment that we have a *testhi insn and the old *tsthi had
> > just 1 alternative:
> >
> >
> > (define_insn "*tsthi"
> >    [(set (cc0)
> >          (compare
> >           (match_operand:ALL2 0 "register_operand"  "r")
> >           (const_int 0)))]
> >   ...)
> >
> >
> > Then the new one would be something like
> >
> >
> > (define_insn "*tsthi"
> >    [(set (cc0)
> >          (compare
> >           (match_operand:ALL2 0 "register_operand"  "r,r")
> >           (const_int 0)))
> >     (clobber (match_scratch:HI 1                   "=0,X")]
> > ...)
> >
> > But how can I express 1) and 2) ?
>
> I don't think you can. Other ports express such transformations using
> define_peephole2 and peep2_reg_dead_p.

This means to duplicate all patterns that currently make use of 
reg_unused_after...  With proposed "dead" attribute there is no need to 
duplicate all patterns.  And it is explicit about the insns which want to use 
that information: the insn itself carries all needed information :-)


> > Currently, my preferred approach is a new drop-in replacement for the old
> > reg_unused_after which uses clobbers to decide whether or not op 0 is still
> > needed.  That way, reg-alloc can work like before and there is no need to
> > implement dozens of new constraint alternatives across the md files.
>
> The problem with this approach is that it may break at random. There's
> just no guarantee that it will work, because you're relying on
> information that just isn't valid anymore.

Would you elaborate on this?

In the proposed solution, the new reg_unused_after just scans the insn pattern 
for clobber of the register of interest, why isn't that clobber valid any more?

If a register is dead after a specific insn, isn't it valid to add a clobber of 
respective regsiter to the insn pattern?  Why does that break at random when 
the pass which adds these clobbers runs before df / cfg is down and has correct 
deadness information availabe?

If some optimization pass, which is filed after that clobber has been added, 
decides to add an insn which uses the contents of that clobbered register, then 
that pass is plain wrong.

The old reg_unused_after has been renamed to avr_reg_unused_after_df and runs 
before cfg goes down, hence is basing its decisions on valid df information -- 
modulo the fact that reg_unused_after is antique code and might need to be 
updated for different reasons.

In principle it should be in order to rely on df at that time (before pass 
free_cfg), isn't it?  Or am I still missing some gory details for why df / cfg 
is invalid and must not be used befor pass *free_cfg for some obscure reason?

Johann

> Unfortunately I don't know enough about CC0-targets to suggest an alternative.
>
> Ciao!
> Steven