[PATCH] Select Elimination in InstCombine

[Gerolf Hoflehner]

Thanks, Eric!   I think I have to rely on dominance optionally to get keep a few of the  MCJIT tests running.

What specifically is on your mind wrt to other queries?

Cheers
Gerolf

On Sep 8, 2014, at 10:14 PM, Eric Christopher <echristo at gmail.com> wrote:

> A few nits in line, one question in line, and one more thought: Can't this replace a few of our other queries for instructions with dominator tree lookups? Is that a follow on patch or?
> 
> Thanks!
> 
> -eric
> 
> ================
> Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2429
> @@ +2428,3 @@
> +//
> +// true when \param SI dominates all of its uses or \param ICmp.
> +//
> ----------------
> s/or/of
> 
> ================
> Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2433
> @@ +2432,3 @@
> +                                    BasicBlock *D) {
> +  bool dominates_all_uses = true;
> +  for (auto I = SI->use_begin(), E = SI->use_end(); I != E; ++I) {
> ----------------
> Local variable naming style.
> 
> ================
> Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2945
> @@ +2944,3 @@
> +
> +        bool transform = false;
> +        SelectInst *SI = cast<SelectInst>(LHSI);
> ----------------
> Local variable naming style.
> 
> ================
> Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2975
> @@ +2974,3 @@
> +            if (ReplaceWithOpd && IsEQorNECompare) {
> +              // replace select with operand on else path for EQ compares.
> +              // replace select with operand on then path for NE compares.
> ----------------
> Nit: Sentences begin with capital letters.
> 
> ================
> Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2977
> @@ +2976,3 @@
> +              // replace select with operand on then path for NE compares.
> +              int Path = I.getPredicate() == ICmpInst::ICMP_EQ ? 1 : 0;
> +              if (InstCombiner::dominatesAllUses(
> ----------------
> Isn't this just a boolean then?
> 
> ================
> Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:95
> @@ -94,1 +94,3 @@
>   AU.addRequired<TargetLibraryInfo>();
> +  AU.addRequired<DominatorTreeWrapperPass>();
> +  AU.addPreserved<DominatorTreeWrapperPass>();
> ----------------
> 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?
> 
> http://reviews.llvm.org/D5258
> 
>