[PATCH] Select Elimination in InstCombine

[Gerolf Hoflehner]

Ok to refactoring in a follow-up commit. 
Is there anything that must be addressed before commit?

Cheers
Gerolf

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

> I still think that we need to confirm there is not a compile time regression from requiring dom trees here...
> 
> Also, some code comments below. I think this code needs to be refactored a bit here. =/ It's already gotten pretty gnarly and this pushes it further.
> 
> Thanks for working on all of this though. I really like the result.
> 
> ================
> Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2428-2431
> @@ -2427,1 +2427,6 @@
> 
> +//
> +// true when \param UI is the only use of \param DI in the parent block and
> +// all other uses of \param DI are in blocks dominated by \param DB.
> +//
> +bool InstCombiner::dominatesAllUses(const Instruction *DI, const Instruction *UI,
> ----------------
> Please follow either the specific coding standards for doxygen comments or match the surrounding code. The function above has a pretty good example.
> 
> ================
> Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2436-2437
> @@ +2435,4 @@
> +         "definition and use must be in the same block");
> +  for (const Use &U : DI->uses()) {
> +    auto *User = cast<Instruction>(U.getUser());
> +    if (User != UI && !(DT && DT->dominates(DB, User->getParent())))
> ----------------
> If all you need is the User, iterate over UI->users()?
> 
> ================
> Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2942
> @@ +2941,3 @@
> +
> +        bool Transform = false;
> +        SelectInst *SI = cast<SelectInst>(LHSI);
> ----------------
> This whole large new block is pretty indicative that this needs to be refactored. I think you should extract a method for the entire transform guarded by this boolean (in a separate patch, should be super simple refactoring).
> 
> The existence of the boolean is a really clear sign that this will help -- you can use early exit rather than a boolean to guard falling through to the transform. It will reduce indentation and make everything much more readable.
> 
> http://reviews.llvm.org/D5258
> 
>