I don't think we should make utilities use AssertingVH like this, at least by default.  Utilities are often built and thrown away.  If it was an analysis, I'd completely agree we should use AssertingVH<div><br><div dir="auto"><br><div class="gmail_quote"><div dir="ltr">On Thu, Jun 29, 2017, 1:33 PM Xin Tong via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">trentxintong added a comment.<br>
<br>
In <a href="https://reviews.llvm.org/D34823#795758" rel="noreferrer" target="_blank">https://reviews.llvm.org/D34823#795758</a>, @dberlin wrote:<br>
<br>
> Without a reason to do this specifically, it seems a waste of time<br>
><br>
> None of this data is used after initial building, and all of these are destroyed after newgvn ends, so updating them doesn't seem to accomplish anything?<br>
<br>
<br>
I was making the Instruction * in OrdereBasicBlock AssertingVH to make sure things are updated properly when instructions are deleted. If they are not, we are essentially holding a dangling pointer which could in worst case result in invalid dominance relationship being returned. Then i found NewGVN is breaking the rule, i.e. erasing an instruction without updating OrderedBasicBlock in OrderedInstruction.<br>
<br>
I agree the way NewGVN is written now is ok even without updating the OrderedBasicBlock. Maybe we should not make Instruction * in OBB AssertingVH. Instead should give user the freedom to invalidate if necessary.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D34823" rel="noreferrer" target="_blank">https://reviews.llvm.org/D34823</a><br>
<br>
<br>
<br>
</blockquote></div></div></div>