<div dir="ltr">This is actually exactly what i feared.<div>"<span style="font-size:12.8px">So it seems we do need to check if a VN exists before adding to LaderTable in addNewInstruction().</span></div>"<div><div>No, it means the leader table is messed up.</div><div><br></div><div>THe check you are trying to add is equivalent to this:</div><div><pre class="" style="font-family:monospace,fixed;font-size:9pt;border:1px solid rgb(196,207,229);padding:4px 6px;margin:4px 8px 4px 2px;overflow:auto;word-wrap:break-word;line-height:15px;color:rgb(0,0,0);background-color:rgb(251,252,253)"><br class="">02382 <span class="" style="color:rgb(128,0,0)">// If the number we were assigned was a brand new VN, then we don't</span>
<a name="l02383" style="color:rgb(61,87,140)"></a>02383 <span class="" style="color:rgb(128,0,0)">// need to do a lookup to see if the number already exists</span>
<a name="l02384" style="color:rgb(61,87,140)"></a>02384 <span class="" style="color:rgb(128,0,0)">// somewhere in the domtree: it can't!</span>
<a name="l02385" style="color:rgb(61,87,140)"></a>02385 <span class="" style="color:rgb(224,128,0)">if</span> (Num >= NextNum) {
<a name="l02386" style="color:rgb(61,87,140)"></a>02386 addToLeaderTable(Num, I, I-><a class="" href="http://llvm.org/docs/doxygen/html/classllvm_1_1Instruction.html#a9cd49851904f15060edb782ef4dd1b2d" style="color:rgb(70,101,162);text-decoration:none">getParent</a>());
<a name="l02387" style="color:rgb(61,87,140)"></a>02387 <span class="" style="color:rgb(224,128,0)">return</span> <span class="" style="color:rgb(0,128,0)">false</span>;
<a name="l02388" style="color:rgb(61,87,140)"></a>02388 }</pre></div><div><br></div><div>As i mentioned, it's doing this because it's not sensible to call findLeader on a new value number.</div><div><br></div><div>Replicating this in the new instruction code fixes nothing. It's just plain wrong ;-)</div><div>In fact, your code does this:<br><div><br></div><div> unsigned Num = VN.lookup_or_add(I);</div><div> uint32_t NextNum = VN.getNextUnusedValueNumber();</div><div><br></div><div>This is the wrong order to call these in ;-)</div><div>You want to get the next unused number before lookup/add it.</div><div><br></div><div>In any case, the bug you have found is that findLeader(VN for ptrtoint, BB) is returning the wrong answer, as I suspect.</div></div><div><br></div><div>It should return %sub.ptr.rhs.cast25 in that BB, but it's returning %0.</div></div><div><br></div><div>This is because the leader table management is a bit broken, and expects to only have a single value per bb (which makes little to no sense if you handle loads, but GVN doesn't so it can get away with it).</div><div><br></div><div>One possible fix i can think of:<br></div><div><br></div><div>Right now you have:<br><br></div><div> // Assign VNs for instructions newly created during GVN optimization.</div><div> void addNewInstruction(Value *Val) {</div><div> if (Instruction *I = dyn_cast<Instruction>(Val)) {</div><div> unsigned Num = VN.lookup_or_add(I);</div><div> uint32_t NextNum = VN.getNextUnusedValueNumber();</div><div> if (Num >= NextNum)</div><div> addToLeaderTable(Num, I, I->getParent());</div><div> }</div><div> }</div><div><br></div><div>change it to </div><div><div> // Assign VNs for instructions newly created during GVN optimization.</div><div> Value *findOrAddNewInstruction(Value *Val) {</div><div> if (Instruction *I = dyn_cast<Instruction>(Val)) {</div><div>// Note: I reversed the order to be correct </div><div> uint32_t NextNum = VN.getNextUnusedValueNumber();</div><div> unsigned Num = VN.lookup_or_add(I);</div><div> if (Num >= NextNum) {<br></div><div> addToLeaderTable(Num, I, I->getParent());</div><div> return nullptr;</div><div> } else if (Value *Existing = findLeader(I->getParent(), Num)) {</div><div> return Existing; </div><div> } else {</div><div> addToLeaderTable(Num, I, I->getParent());</div><div> }</div><div> }</div><div> }</div></div><div><br></div><div><br></div><div>Call add new instruction, and if it returns non null, replace the instruction you created with the return value (or GVN will do it later for you if you like).</div><div><br></div><div>If you'd rather have GVN do that part, change it to:<br><div> void addNewInstruction(Value *Val) {</div><div> if (Instruction *I = dyn_cast<Instruction>(Val)) {</div><div> // Note: I reversed the order to be correct </div><div> uint32_t NextNum = VN.getNextUnusedValueNumber();</div><div> unsigned Num = VN.lookup_or_add(I);</div><div> if (Num >= NextNum) {<br></div><div> addToLeaderTable(Num, I, I->getParent());</div><div> } else if (findLeader(I->getParent(), Num) != nullptr) {</div><div> return; </div><div> } else {</div><div> addToLeaderTable(Num, I, I->getParent());</div><div> }</div><div> }</div><div> }</div></div><div><br></div><div>GVN will do the propagation for you, at the expense of extra findLeader calls</div><div><br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Nov 17, 2015 at 10:43 AM, Weiming Zhao <span dir="ltr"><<a href="mailto:weimingz@codeaurora.org" target="_blank">weimingz@codeaurora.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">weimingz added a comment.<br>
<br>
Hi Berlin,<br>
<br>
Here is what’s happening:<br>
<br>
When it sees the load in BB if.then.14:<br>
<br>
%v1 = load i32, i32* bitcast (i8** @dfg_text to i32*), align 4<br>
<br>
Then, it tries to do PRE.<br>
So it finds the store in BB while.end:<br>
<span class=""><br>
store i8* %add.ptr, i8** @dfg_text, align 4<br>
<br>
</span>So, in GetStoreValueForLoad, it inserts a PtrToInt (line 1153)<br>
<br>
Now, the BB becomes<br>
<br>
while.end: ; preds = %while.cond.16<br>
<br>
%add.ptr = getelementptr inbo %sub.ptr.rhs.cast25 unds i8, i8* %v2, i32 undef<br>
<span class=""> store i8* %add.ptr, i8** @dfg_text, align 4<br>
</span> %sub.ptr.rhs.cast25 = ptrtoint i8* %add.ptr to i32 *** original ptrtoint ***<br>
%sub.ptr.sub26 = sub i32 0, %sub.ptr.rhs.cast25<br>
%0 = ptrtoint i8* %add.ptr to i32 *** newly created ***<br>
<span class=""> switch i32 undef, label %sw.default [<br>
<br>
</span>This new instruction happens to have the same GVN as %sub.ptr.rhs.cast25<br>
<br>
So, when GVN process BB while.end, if finds that %sub.ptr.rhs.cast25 already have a GVN, so deleted it and replaces the use with %0, which is wrong.<br>
<br>
So it seems we do need to check if a VN exists before adding to LaderTable in addNewInstruction().<br>
<br>
<br>
<a href="http://reviews.llvm.org/D14670" rel="noreferrer" target="_blank">http://reviews.llvm.org/D14670</a><br>
<br>
<br>
<br>
</blockquote></div><br></div>