<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 10 August 2015 at 13:53, Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">dberlin added a comment.<br>
<br>
So, this is essentially trying to start to get GVN to recognize that operations are made of operations on value numbers.<br>
<br>
Except you are only handling the really special case where those value numbers are  constant over the entire program</blockquote><div><br></div><div>If you're referring to the new logic around <span style="font-size:12.8000001907349px">ReplaceWithConstMap, it's assuming</span> those values are constant for a single block, not the entire program.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> (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)<br></blockquote><div><br></div><div>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. :-/</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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 :)<br>
<br>
So SGTM<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/GVN.cpp:2231<br>
@@ +2230,3 @@<br>
<span class="">+      if (ICmpInst *ICmpI = dyn_cast<ICmpInst>(V)) {<br>
</span><span class="">+        if (ICmpI->getSignedPredicate() == ICmpInst::Predicate::ICMP_EQ) {<br>
+<br>
</span>----------------<br>
Why only ICMP_EQ?<br>
<span class=""><br>
================<br>
Comment at: lib/Transforms/Scalar/GVN.cpp:2245<br>
@@ +2244,3 @@<br>
+                // equality propagation can't be done for every<br>
+                // successor, but propagateEquality checks it<br>
+                Changed |= propagateEquality(Lhs,<br>
----------------<br>
</span>nlewycky wrote:<br>
> Period.<br>
This isn't quite right :)<br>
There are cases that propagate equality checks, and cases we don't call it because it would do the wrong thing.<br>
<br>
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).<br>
<br>
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)<br></blockquote><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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.<br>
<br>
<br>
<br>
<br>
<a href="http://reviews.llvm.org/D11918" rel="noreferrer" target="_blank">http://reviews.llvm.org/D11918</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>