Re: [PATCH][RTL-ifcvt] Make non-conditional execution if-conversion more aggressive

[Kyrill Tkachov <kyrylo dot tkachov at arm dot com>]

On 27/07/15 17:09, Jeff Law wrote:
> On 07/27/2015 04:17 AM, Kyrill Tkachov wrote:
> > I experimented with resource.c and the roadblock I hit is that it
> > seems to have an assumption that it operates on hard regs (in fact
> > the struct it uses to describe the resources has a HARD_REG_SET for
> > the regs) and so it triggers various HARD_REGISTER_P asserts when I
> > try to use the functions there. if-conversion runs before register
> > allocation, so we're dealing with pseudos here.
> Sigh.  resource.c probably isn't going to be useful then.
>
> > My other attempt was to go over BB_A and mark the set registers in a
> >   bitmap then go over BB_B and do a FOR_EACH_SUBRTX of the SET_SRC of
> > each insn. If a sub-rtx is a reg that is set in the bitmap from BB_A
> > we return false. This seemed to do the job and testing worked out ok.
> > That would require one walk over BB_A, one walk over BB_B but I don't
> > know how expensive FOR_EACH_SUBRTX walks are...
> >
> > Would that be an acceptable solution?
> I think the latter is reasonable.  Ultimately we have to do a full look
> at those rtxs, so it's unavoidable to some extent.
>
> The only other possibility would be to use the DF framework.  I'm not
> sure if it's even initialized for the ifcvt code.  If it is, then you
> might be able to avoid some of the walking of the insns and instead walk
> the DF structures.

I think it is initialized (I look at df_get_live_out earlier on
in the call chain). I suppose what we want is for the live in regs for BB_B
to not include any of the set regs in BB_A?


> > <snip>
> > > It fails when the last insn is not recognised, because
> > > noce_try_cmove_arith can modify the last insn, but I have not seen
> > > it cause any trouble. If it fails then back in noce_try_cmove_arith
> > > we goto end_seq_and_fail which ends the sequence and throws it away
> > > (and cancels if-conversion down that path), so it should be safe.
> > > OK, I was working for the assumption that memoization ought not
> > > fail, but it seems that was a bad assumption on my part.    So
> > > given noce_try_cmove_arith can change the last insn and make it
> > > no-recognizable this code seems reasoanble.
> > >
> > > So I think the only outstanding issues are:
> > >
> > > 1. Investigate moving rather than re-emitting insns.
> > I'll look into that, but what is the machinery by which one moves
> > insns?
> I don't think we have any good generic machinery for this.  I think
> every pass that needs this capability unlinks the insn from the chain
> and patches it back in at the new location.

That's the SET_PREV_INSN, SET_NEXT_INSN functions, right?

The current way the top-level noce_process_if_block is structured
it expects the various ifcvt functions (like noce_try_cmove_arith)
to generate a sequence, then it takes it, unshares it and removes
the empty basic blocks.

If we're to instead move insns around we'd need to further modify
 noce_process_if_block to handle differently
 this one case where we move insns instead of re-emitting them.
I think this would make that function more convoluted than it needs to be.
With the current approach we always call unshare_all_rtl_in_chain on the
emitted sequence which should take care of any RTL sharing issues and in
practice I don't expect to have more than 3-4 insns in these sequences since
they will be guarded by the branch cost.

So I would rather argue for re-emitting insns in this case to keep consistent
with the dozen or so similar functions in ifcvt.c that already work that way.

Thanks,
Kyrill

> jeff