[PATCH] Select Elimination in InstCombine

[Hal Finkel]

----- Original Message -----
> From: "Juergen Ributzka" <juergen at apple.com>
> To: "Gerolf Hoflehner" <ghoflehner at apple.com>
> Cc: "LLVM Commits" <llvm-commits at cs.uiuc.edu>, reviews+D5258+public+f9996d3e9899d3c8 at reviews.llvm.org
> Sent: Tuesday, September 9, 2014 4:55:29 PM
> Subject: Re: [PATCH] Select Elimination in InstCombine
> 
> 
> The problem with making DT optional is that you might never get it.
> If any pass that runs before InstCombine invalidates DT, it won’t be
> available to any following pass until there is a pass that
> explicitly requires it.

That's true. There have been objections in the past to making it required, and I don't recall by whom or why exactly. In the current standard pass manager configuration, it does happen at least once.

 -Hal

> 
> 
> -Juergen
> 
> 
> 
> 
> 
> 
> On Sep 9, 2014, at 2:50 PM, Gerolf Hoflehner < ghoflehner at apple.com >
> wrote:
> 
> Ok, makes sense. The DT is optional and I added the missing check.
> When it is absent we just don’t recognize as many patterns.
> 
> I expect DT usage to evolve over time, but have no specifics plans.
> 
> -Gerolf
> 
> On Sep 9, 2014, at 12:52 AM, Eric Christopher < echristo at gmail.com >
> wrote:
> 
> 
> 
> Hi Gerolf,
> 
> One more comment in line.
> 
> As far as the dominance tree, you don't ever check to find out if you
> have dominance trees, you just require it. The leading question was:
> if it's not optional, then you can probably use it in more places in
> the pass. If it is optional then you're not actually checking for it
> before using it.
> 
> Thanks!
> 
> ================
> 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(
> ----------------
> majnemer wrote:
> 
> 
> echristo wrote:
> 
> 
> Isn't this just a boolean then?
> I'd make this an unsigned, getSuccessor is asking which successor to
> take. It just so happens that we are only interested in cases where
> there are two successors.
> Yes. Makes sense that way. Might be better off just grabbing the
> correct successor rather than mucking with the integer.
> I agree.
> 
> 
> 
> http://reviews.llvm.org/D5258
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory