Re: Add a new combine pass

[Richard Sandiford <richard dot sandiford at arm dot com>]

Segher Boessenkool <segher@kernel.crashing.org> writes:
> Hi!
>
> On Mon, Nov 25, 2019 at 09:16:52PM +0000, Richard Sandiford wrote:
>> Segher Boessenkool <segher@kernel.crashing.org> writes:
>> > I am wondering the other way around :-)  Is what you do for combine2
>> > something that would be more generally applicable/useful?  That's what
>> > I'm trying to find out :-)
>> >
>> > What combine does could use some improvement, if you want to hear a
>> > more direct motivations.  LOG_LINKS just skip references we cannot
>> > handle (and some more), so we always have to do modified_between etc.,
>> > which hurts.
>> 
>> The trade-offs behind the choice of representation are very specific
>> to the pass.
>
> Yes, but hopefully not so specific that every pass needs a completely
> different representation ;-)

Well, it depends.  Most passes make do with df (without DU/UD-chains).
But since DU/UD-chains are naturally quadratic in the general case,
and are expensive to keep up to date, each DU/UD pass is going to have
make some compromises.  It doesn't seem too bad that passes make
different compromises based on what they're trying to do.  (combine:
single use per definition; fwprop.c: track all uses, but for dominating
definitions only; sched: fudged via a param; regrename: single
definition/multiple use chains optimised for renmaing; combine2: full
live range information, but limited use list; etc.)

So yeah, if passes want to make roughly the same compromises, it would
obviously be good if they shared a representation.  But since each pass
does something different, I don't think it's a bad sign that they make
different compromises and use different representations.

So I don't think a new pass with a new representation is in itself a
sign of failure.

>> >> >> Target                 Tests   Delta    Best   Worst  Median
>> >> >> avr-elf                 1341 -111401  -13824     680     -10
>> >> >
>> >> > Things like this are kind of suspicious :-)
>> >> 
>> >> Yeah.  This mostly seems to come from mopping up the extra moves created
>> >> by make_more_copies.  So we have combinations like:
>> >> 
>> >>    58: r70:SF=r94:SF
>> >>       REG_DEAD r94:SF
>> >>    60: r22:SF=r70:SF
>> >>       REG_DEAD r70:SF
>> >
>> > Why didn't combine do this?  A target problem?
>> 
>> Seems to be because combine rejects hard-reg destinations whose classes
>> are likely spilled (cant_combine_insn_p).
>
> Ah, okay.  And that is required to prevent ICEs, in combine2 as well
> then -- ICEs in RA.

Not in this case though.  The final instruction is a hardreg<-pseudo move
whatever happens.  There's nothing special about r70 compared to r94.

> There should be a better way to do this.

ISTM we should be checking for whichever cases actually cause the
RA failures.  E.g. to take on extreme example, if all the following
are true:

- an insn has a single alternative
- an insn has a single non-earyclobber output
- an insn has no parallel clobbers
- an insn has no auto-inc/decs
- an insn has a hard register destination that satisfies its constraints
- the hard register is defined in its original location

then there should be no problem.  The insn shouldn't need any output
reloads that would conflict with the hard register.  It also doesn't
extend the live range of the output.

Obviously that's a lot of conditions :-)  And IMO they should be built
up the other way around: reject specific cases that are known to cause
problems, based on information about the matched insn.  But I think the
avr example shows that there's a real problem with using REGNO_REG_CLASS
for this too.  REGNO_REG_CLASS gives the smallest enclosing class, which
might not be the most relevant one in context.  (It isn't here, since
we're just passing arguments to functions.)

>> This SF argument register
>> happens to overlap POINTER_X_REGS and POINTER_Y_REGS and so we reject
>> the combination based on POINTER_X_REGS being likely spilled.
>
> static bool
> avr_class_likely_spilled_p (reg_class_t c)
> {
>   return (c != ALL_REGS &&
>            (AVR_TINY ? 1 : c != ADDW_REGS));
> }
>
> So this target severely shackles combine.  Does it have to?  If so, why
> not with combine2?

As far as the above example goes, I think returning true for
POINTER_X_REGS is the right thing to do.  It only has two 8-bit
registers, and they act as a pair when used as a pointer.

Thanks,
Richard