[PATCH] D34823: [PredicateInfo] Invalidate an OrderedBasicBlock when instruction in it is deleted.

[Daniel Berlin via llvm-commits]

Yes.
If it was an Analysis, it could be preserved.
As a utility, it isn't.


On Thu, Jun 29, 2017 at 1:43 PM, Xin Tong <trent.xin.tong at gmail.com> wrote:

> Because analysis might be used by later passes ?
> -Xin
>
> On Fri, Jun 30, 2017 at 5:35 AM, Daniel Berlin <dberlin at dberlin.org>
> wrote:
> > 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
> >
> >
> > On Thu, Jun 29, 2017, 1:33 PM Xin Tong via Phabricator
> > <reviews at reviews.llvm.org> wrote:
> >>
> >> trentxintong added a comment.
> >>
> >> In https://reviews.llvm.org/D34823#795758, @dberlin wrote:
> >>
> >> > Without a reason to do this specifically, it seems a waste of time
> >> >
> >> > 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?
> >>
> >>
> >> 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.
> >>
> >> 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.
> >>
> >>
> >> https://reviews.llvm.org/D34823
> >>
> >>
> >>
> >
>
>
>
> --
> Software Engineer - Compiler Toolchain
> Employee of Facebook Inc.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170629/c0552cd9/attachment.html>