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

[Jeff Law <law at redhat dot com>]

On 07/27/2015 10:40 AM, Kyrill Tkachov wrote:
> 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?
I was thinking more that you'd walk BB_A, which would give you a set of 
registers changed in BB_A.  Then you could walk the uses of those regs 
using the DF structures and see if any are in BB_B.

That may not be any better in practice.  It'd depend on things like how 
many uses exist outside BB_B vs the size of BB_B.  Perhaps it isn't 
worth the trouble and we should stick with a single walk of both blocks?


> > 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?
Right.  Passes would just unlink the insn from the chain, then link it 
back in at a new location.  It's always been fairly ad-hoc at the RTL level.

> 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.
OK, then let's stay with re-emitting since that's the structure 
generally expected elsewhere in ifcvt.c.  I just get worried anytime I 
spot the potential for invalid RTL sharing.


Jeff