[PATCH] D66190: [CodeGen] Add a pass to do block predication on SSA machine IR

Wed Aug 14 08:09:09 PDT 2019

ThomasRaoux added a comment.

In D66190#1628864 <https://reviews.llvm.org/D66190#1628864>, @lebedev.ri wrote:

> One more question - i can't recall if there's precedent for bundling more than one pass
>  into the same source file, that does not look ideal.

Looking quickly at it, I see few cases (ex: https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/MachineInstrBundle.cpp) but doesn't mean it is a great idea.
The alternative would be to move the SSAIfConv helper class in a header file.  Do you think this is a better solution?

> Can the pass not be run standalone, does it really require some specific target?
>  Perhaps that can be short-circuited via some `cl::opt` just for testing purposes?

It can be ran standalone but unless the target has support for predicate and implements the canInsertSelect hook it won't do anything. It needs both features to do the transformation.

> I'm not sure it's a great idea to add dead code with no upstream test coverage.
>  At best, it will silently regress, at worst it can simply be dropped as dead code.

I agree. If moving the SSAIfConv as a helper that can be reused is an acceptable solution, then I can do that and then it is easy to move the pass as is. Otherwise I'll probably copy this code for now until we have a target supporting the required features.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66190/new/

https://reviews.llvm.org/D66190