[PATCH] D38313: [InstCombine] Introducing Pattern Instruction Combine plug-in into InstCombine pass

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D38313#906667, @aaboud wrote:

> In https://reviews.llvm.org/D38313#906511, @spatel wrote:
>
> > In https://reviews.llvm.org/D38313#906324, @aaboud wrote:
> >
> > > Saying that, I believe that running all current instcombine tests with this new functionality is a must, in order to make that possible, it is obvious that we need to be part of instcombine pass.
> > >
> > > Note that the implementation is done in a way that moving it to a separate pass can be done with zero effort, but in a cost of ignoring/dropping few hundreds of LIT tests.
> >
> >
> > I agree that testing must be done, but I don't see how that makes it obvious that this should be part of instcombine? If you're concerned that something else in instcombine will inhibit or invert this transform, you could add tests under test/Transforms/PhaseOrdering/ to make sure that doesn't happen. I think you've done the hard part (the code itself) already. :)
> >
> > The major disadvantage of being in instcombine is that this code will be running 5-6 times in a typical pipeline when it probably doesn't need to.
>
>
> Is that a bad thing to run this code 5-6 times? it has O(n) complexity where n = number of instructions in the function.
>  Would not it catch more patterns once we run other optimizations? 
>  I need this code to run at least twice, one time on regular compilation and another time as part of LTO optimization.
>  Would it be better if I move the code to a new pass "PatternInstructionCombinePass", but run that new pass from "addInstructionCombiningPass"? It will still run 5-6 times!
>
> >> Do you think still that it should be in a separate pass?
> > 
> > I don't know enough to say. I'm stating a concern based on feedback I've gotten about the size and cost of InstCombine (eg, http://lists.llvm.org/pipermail/llvm-dev/2017-May/113184.html , http://lists.llvm.org/pipermail/llvm-dev/2017-September/117151.html ). I'll let more experienced developers (cc'ing @nlopes @hfinkel @regehr ... ) decide what's the right way forward.
> > 
> > Also, I know that others are exploring ways to auto-regenerate some portion of InstCombine for greater efficiency, so it might be worth considering if/how that goal interacts with a large patch like this one.
>
> Notice that this code is part of InstCombine pass, but it is totally separated, I do not see why any change to InstCombine will affect this, unless you think we can auto-generate this optimization!
>
> To summarize, I really do not mind to move it to separate pass,


I think that this should be a separate pass. Everything in InstCombine is currently part of InstCombine's fixed-point iteration scheme. Having some things in InstCombine that are, and some that aren't, seems unnecessarily confusing. If this essentially runs as a separate pass, then I think we should just make it one, and then we can schedule it as we see fit.

Also, I'll note that this optimization seems very similar to PPCTargetLowering::DAGCombineTruncBoolExt, which optimized cases like `trunc(binary-ops(binary-ops(zext(x), zext(y)), ...)` and perhaps also  PPCTargetLowering::DAGCombineExtBoolTrunc, which optimizes cases like `zext(binary-ops(binary-ops(trunc(x), trunc(y)), ...)`. Those implementation are not recursive, but use a work queue, and I suggest doing the same here.

> but I would like to make this optimization committed as soon as possible.
>  I appreciate your review and direction, please, advice with the best way you think I should implement this optimization.
> 
> Thanks,
> Amjad




https://reviews.llvm.org/D38313