[PATCH] D11918: Constant propagation after hiting assume(icmp)

[Nick Lewycky via llvm-commits]

On 10 August 2015 at 13:53, Daniel Berlin <dberlin at dberlin.org> wrote:

> dberlin added a comment.
>
> So, this is essentially trying to start to get GVN to recognize that
> operations are made of operations on value numbers.
>
> Except you are only handling the really special case where those value
> numbers are  constant over the entire program


If you're referring to the new logic around ReplaceWithConstMap, it's
assuming those values are constant for a single block, not the entire
program.

(instead of also handling cases where the operations simplify, or have
> arithmetic niceties, etc. SimplifyInstruction will do some of this, but it
> won't notice some classes of things)
>

Right. The alternative is trying to update the whole BasicBlockEdge based
system of leader numbers to handle learning new facts mid-block instead of
along edges. :-/

I'm generally biased against trying to bolt more into our current GVN, but
> hey, when you do stuff like this, it just makes New GVN a lot faster by
> comparison :)
>
> So SGTM
>
>
> ================
> Comment at: lib/Transforms/Scalar/GVN.cpp:2231
> @@ +2230,3 @@
> +      if (ICmpInst *ICmpI = dyn_cast<ICmpInst>(V)) {
> +        if (ICmpI->getSignedPredicate() == ICmpInst::Predicate::ICMP_EQ) {
> +
> ----------------
> Why only ICMP_EQ?
>
> ================
> Comment at: lib/Transforms/Scalar/GVN.cpp:2245
> @@ +2244,3 @@
> +                // equality propagation can't be done for every
> +                // successor, but propagateEquality checks it
> +                Changed |= propagateEquality(Lhs,
> ----------------
> nlewycky wrote:
> > Period.
> This isn't quite right :)
> There are cases that propagate equality checks, and cases we don't call it
> because it would do the wrong thing.
>
> For example, i know your code doesn't quite work right with assume
> followed by certain types of switch statements (where there are multiple
> edges to the same successor).
>
> See the code handling switch statements, that verifies there are not
> multiple edges to the same successor, and if there are not, *does not call
> propagateEquality* (old line 2259)
>

That's just saying that if a switch has x == 7 goto block A and x == 9 goto
block A, don't propagate x = 7 or x = 9 to block A. In this case we don't
have two different contradictory facts, it has a single assume that has
been executed and we can propagate that to all dominated instructions.

You are going to either need to unify or duplicate a lot of this kind of
> logic, depending on the terminator of the basic block the assume statement
> is in.
>
>
>
>
> http://reviews.llvm.org/D11918
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150810/18976fae/attachment.html>