<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 4, 2017 at 9:39 PM, Sanjoy Das via Phabricator via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">sanjoy requested changes to this revision.<br>
sanjoy added a comment.<br>
This revision now requires changes to proceed.<br>
<br>
Comments inline.<br>
<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/<wbr>EarlyCSE.cpp:620<br>
<span class="">+    // kicks in when we've inferred a control dependent equivelence fact.  Note<br>
+    // that this does make the algorithm O(I^2) in the worst case (a series of<br>
+    // calls which each use the previous instructions), but this was already<br>
</span>----------------<br>
In the pathological case that you mentioned won't this be O(I^3), since you're doing:<br>
<br>
```<br>
for each Inst in BB:<br>
  for each operand I of Inst:<br>
    for each operand X of I:<br>
      hash(X)<br>
```<br>
<br>
Maybe we can find an better bound as a function of the number of total //uses//?<br></blockquote><div><br></div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div><br></div><div><br></div><div>[1] For example</div><div><br></div><div>if (i == j)</div><div>  if (i != j)</div><div><br></div><div>The inner if will not be eliminated by EarlyCSE, as it only handles predicate swapping, it doesn't take  i == j and insert</div><div><br></div><div>i == j = true</div><div>i != j == false</div><div>i < j == false</div><div>i > j == false</div><div>into the equivalence table (even though it's probably cheap)</div><div>etc</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/<wbr>EarlyCSE.cpp:674<br>
<span class="">+          DEBUG(dbgs() << "EarlyCSE CVP: Add conditional value for '"<br>
+                << CondI->getName() << "' as true after "<br>
+                << Inst->getName()  << "\n");<br>
</span>----------------<br>
This bit is NFC right (just adds the debug output)?  If so, I'd just land this without review.<br>
<br>
<br>
================<br>
Comment at: test/Transforms/EarlyCSE/<wbr>guards.ll:195<br>
<span class="">+  %cond0 = icmp slt i32 %val, 40<br>
+  call void(i1,...) @llvm.experimental.guard(i1 %cond0) [ "deopt"() ]<br>
+  call void(i1,...) @llvm.experimental.guard(i1 %cond0) [ "deopt"() ]<br>
</span>----------------<br>
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.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D28275" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D28275</a><br>
<br>
<br>
<br>
______________________________<wbr>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div>