[llvm-commits] [patch] Add a pass for duplicating indirectbr at the IL level

[Rafael Ávila de Espíndola]

On 11-07-22 2:44 AM, Bob Wilson wrote:
> I've also been working on this problem.  I've resurrected the work I
> had started last year to have a smarter strategy for duplicating
> indirect branches.  I don't have anything ready to commit yet, but
> it's coming along (slowly).  I'm trying to solve a broader set of
> problems than you are tackling with this patch.  For example, to pick
> just one, your new pass can still only duplicate past unconditional
> branches.  That's not good enough for something like JavaScriptCore.
> You also have the same problems as the existing pass with excessive
> duplication.  But, since it might take me a while to finish my work,
> I won't have any objections to taking something like this patch if it
> doesn't conflict with where I think we should be heading in the
> longer term.

I agree that the patch is not the final state. Hopefully it is going
in the right direction, as for example the logic for updating ssa
info can be reused. In fact, my plan with the patch was to get the
smallest usable change and move from there. Some of my ideas for
it are

* Add support for duplicating any small (1 instruction?) bb. This
would make the old IL taildup pass fully redundant.

* Add support for duplicating "simple" bb to conditional branches.
With pr10254 fixed it should make the MI level taildup fully redundant.

BTW, what is your plan for duplicating into a conditional branch? I
would be interested in implementing that too...

> It looks to me like you've basically taken the existing MI-level
> pass, moved it to the IL-level and added a special-case check for
> whether the indirect branch address is coming from a load.  What if
> it's not a load?

Take the "shouldDuplicate" logic as experimental. That worked for the
case I had. The idea is that it was something that duplicating could
simplify (phi of load of a phi) by duplicating.

I still have to test it a bit more.

> I'm assuming that you're doing this to expose the
> load in the selection DAG, right?  An alternative might be a
> target-specific peephole optimization to merge the load with the
> indirect branch at the MI level?

That could work, but it would have to happen before the register
allocator for best results. I did this at the IL level because of
the nice results I got with my old patch "duplicating" (i.e. not
merging) in clang.

> One big concern I have is that this patch would be forcing us to have
> two separate implementations of the SSA updates, one here at the IL
> level and another in the existing MI level early tail dup pass.

My hope is to make the one on MI redundant.

 > As
> you should remember, I was pretty hesitant to go along with your
> recent changes to the early tail dup pass, because I was hoping to
> remove it entirely, thereby removing all the SSA complexity from tail
> duplication.

I hope to remove the one in CodeGen. I disagree that the complexity
involved is particularly bad. The IL version is simpler and if we can 
fix one of the "FIXME" in SSAUpdater itself, it will be
down to one corner case (adding an available value when we don't 
duplicate), which is on the same level of things like simplifycfg.

 >  But, since you showed that there were some significant
> benefits, I agreed that it was the right thing to do, with the hope
> that we could share the SSA updating code between early tail dup and
> the smarter indirect branch dup pass that I'm working on.  I suppose
> we might be able to templatize that SSA updating code in the same way
> that the SSAUpdater is templatized to work at both levels, but that's
> a big hassle all around and I'd really like to avoid it.

Yes, that is my plan B: do the same that was done for SSAUpdate and
create IL and MI traits and move the high level logic to a
template.

Thanks,
Rafael