[PATCH] D69914: [LVI] Normalize pointer behavior

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D69914#2107451 <https://reviews.llvm.org/D69914#2107451>, @nikic wrote:

> Thanks to D82261 <https://reviews.llvm.org/D82261> GetUnderlyingObject() is now cheap enough than caching is not worth the bother. Most of the remaining cost there will be addressed by D81867 <https://reviews.llvm.org/D81867>. New compile-time numbers: http://llvm-compile-time-tracker.com/compare.php?from=37d3030711cc30564fb142154e4e8cabdc91724e&to=e518ba98b7fb90f2645b91edd8cdeae40ad6f4df&stat=instructions I profiled the sqlite case and found that while LVI cost does increase a bit, the main contribution are second-order effects. As such I'm happy enough with performance to reapply this.
>
> In D69914#2106549 <https://reviews.llvm.org/D69914#2106549>, @fhahn wrote:
>
> > In D69914#2105255 <https://reviews.llvm.org/D69914#2105255>, @nikic wrote:
> >
> > > Furthermore this change seems to have a larger impact on optimization than I expected, with larger code-size changes for some benchmarks: https://llvm-compile-time-tracker.com/compare.php?from=f87b785abee0da8939fdd5900a982311b4c25409&to=f3490beadb768e921e531fd61450a7c5bfa84f2d&stat=size-text Apparently being able to eliminate a null branch at the end of the block where the pointer is dereferenced is quite valuable.
> >
> >
> > Interesting! I am wondering if it might be worth to handle simplifications based on dereferenceability in a separate pass, rather than making the caching more complicated. I am not sure how much benefit we get from other places in LVI knowing that a ptr is not null, but I would expect it not too be too much.
> >
> > I've put up a simple prototype that just keeps track of dereferenced objects based on dominance D82299 <https://reviews.llvm.org/D82299> and eliminates null checks in a single traversal of a sorted list of dereferences/compares.
>
>
> Thanks for looking into this! Some thoughts on the tradeoff between a separate pass and having this in LVI:
>
> - LVI gets nonnull information from three sources: isKnownNonZero for basic cases, pointer dereferences, and pointer comparisons. One important aspect here is that this information is combined, and not limited to dominating cases only. For example, if you have two incoming edges and the pointer is dereferenced in both predecessor blocks, LVI knows that is is non-null (even though neither dereference is dominating). Similarly, if the pointer is dereferenced in one predecessor, but the other one is guarded by a !== null check, we still get the same result.
> - CVP uses LVI nonnull information both for compares and for call nonnull attributes. Handling those two in a separate pass is easy.
> - JumpThreading is unfortunately outside my area of familiarity, but I believe this is the part where having nonnull-reasoning inside LVI rather than separately is important. Jump threading does a lot of reasoning about predicates on edges and queries LVI to do so. Dropping this functionality from LVI would probably make threading of !== null less effective.


The last point can indeed not really be solved by a separate patch unfortunately. I also won't have much time to look into the separate patch in the near future, so I don't think we should wait with this patch until then.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69914/new/

https://reviews.llvm.org/D69914