[PATCH] Select Elimination in InstCombine

[Gerolf Hoflehner]

On Sep 9, 2014, at 3:01 PM, Chandler Carruth <chandlerc at gmail.com> wrote:

> (most of the other reviewers are doing a great job on this patch, so I've only made a couple of specific comments... will leave the rest of the review to them)
> 
> ================
> Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2431
> @@ +2430,3 @@
> +//
> +bool InstCombiner::dominatesAllUses(SelectInst *SI, ICmpInst &ICmp,
> +                                    BasicBlock *D) {
> ----------------
> joey wrote:
>> I'm wondering if this name is too general, or if the parameter types are too strict.
> Agreed. I Think the name is good, but it should be made a very generic property of an instruction.
> 
> I actually wonder whether this should be a method on DominatorTree itself.

It is a specific property of the instruction. So I would not consider is a method of DT. I make the interface more generic for now. The function certainly is a utility routine, so I’m very much open to find it a better home than the instruction combiner. The property it checks is: 
for a given instruction is there exactly one use (and this use must be in a given second instruction) in the same block and are all other uses dominated by a given block?
> 
> ================
> Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:95
> @@ -94,1 +94,3 @@
>   AU.addRequired<TargetLibraryInfo>();
> +  AU.addRequired<DominatorTreeWrapperPass>();
> +  AU.addPreserved<DominatorTreeWrapperPass>();
> ----------------
> echristo wrote:
>> Probably don't need to have it as requires, but optional? i.e. if it doesn't exist then just don't do the transform in the way you've got?
> I'm actually OK changing InstCombine so that it always requires dominator trees, but I think that should be a separate change.
> 
> I would add it as optional here, and then I would submit a follow-up patch to switch it over so we don't hold up the logic here on the debate about how often to require the analysis. Then the patch that makes it required can discuss the relevant concern by citing compile-item benchmarks showing it's not too expensive. =]
> 
> http://reviews.llvm.org/D5258
> 
>