[PATCH] D28275: [EarlyCSE] infer conditional equalities within basic blocks

[Daniel Berlin via llvm-commits]

On Wed, Jan 4, 2017 at 9:39 PM, Sanjoy Das via Phabricator via llvm-commits
<llvm-commits at lists.llvm.org> wrote:

> sanjoy requested changes to this revision.
> sanjoy added a comment.
> This revision now requires changes to proceed.
>
> Comments inline.
>
>
>
> ================
> Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:620
> +    // kicks in when we've inferred a control dependent equivelence
> fact.  Note
> +    // that this does make the algorithm O(I^2) in the worst case (a
> series of
> +    // calls which each use the previous instructions), but this was
> already
> ----------------
> In the pathological case that you mentioned won't this be O(I^3), since
> you're doing:
>
> ```
> for each Inst in BB:
>   for each operand I of Inst:
>     for each operand X of I:
>       hash(X)
> ```
>
> Maybe we can find an better bound as a function of the number of total
> //uses//?
>


Yes, unless something crazy is going on here,  the set of things you could
ever find a hit for is limited to the set of operands that appear in the
equivalences we've entered into the table.  In the case of EarlyCSE, which
only adds an equivalence for the condition itself (and not variants)[1],
the only affected instructions are uses of conditional.

At worst, you know those uses at the time you enter them into the table,
and can mark the instructions you should be processing here, so that you
process them at the "right" time by just testing if they are in the
"toprocess" set.



[1] For example

if (i == j)
  if (i != j)

The inner if will not be eliminated by EarlyCSE, as it only handles
predicate swapping, it doesn't take  i == j and insert

i == j = true
i != j == false
i < j == false
i > j == false
into the equivalence table (even though it's probably cheap)
etc


>
> ================
> Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:674
> +          DEBUG(dbgs() << "EarlyCSE CVP: Add conditional value for '"
> +                << CondI->getName() << "' as true after "
> +                << Inst->getName()  << "\n");
> ----------------
> This bit is NFC right (just adds the debug output)?  If so, I'd just land
> this without review.
>
>
> ================
> Comment at: test/Transforms/EarlyCSE/guards.ll:195
> +  %cond0 = icmp slt i32 %val, 40
> +  call void(i1,...) @llvm.experimental.guard(i1 %cond0) [ "deopt"() ]
> +  call void(i1,...) @llvm.experimental.guard(i1 %cond0) [ "deopt"() ]
> ----------------
> This will get the case where the second guard was using `%cond1 = <same
> expression as %cond0>` right?  If so, please add a test case that checks
> that.
>
>
> https://reviews.llvm.org/D28275
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170104/05b9c84f/attachment.html>