[PATCH] D84181: [GVN] Rewrite IsValueFullyAvailableInBlock()

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri updated this revision to Diff 280271.
lebedev.ri added a comment.

In D84181#2168983 <https://reviews.llvm.org/D84181#2168983>, @lebedev.ri wrote:

> In D84181#2168819 <https://reviews.llvm.org/D84181#2168819>, @fhahn wrote:
>
> > This overall makes sense I think.
> >
> > In D84181#2168702 <https://reviews.llvm.org/D84181#2168702>, @lebedev.ri wrote:
> >
> > > In D84181#2168668 <https://reviews.llvm.org/D84181#2168668>, @mkazantsev wrote:>
> > >
> > > > I'd suggest the following reliable way to make sure it is NFC:
> > > >
> > > > - Instead of changing logic of `IsValueFullyAvailableInBlock`, introduce a new version of it and use it.
> > > > - In debug mode, also call the old one and assert that the results are the same.
> > > > - Then, after it has been in for a long enough while and we are certain those algorithms do the same thing, remove the old one.
> > > >
> > > >   WDYT?
> > >
> > >
> > > I'm afraid it's not that easy to do.
> > >
> > > 1. We'd need to workaround the situation where the old code gave up due to the `gvn-max-recurse-depth` recursion limit
> >
> >
> > I think we also need some kind of limit on the worklist size for the iterative algorithm.  Without it we might traverse the whole function in some cases, which could lead to huge compile-times in some degenerate cases I think.
>
>
> Okay, added one. Right now it's based on the number of times we find a previously unknown block,
>  mark it as speculatively available, and recurse into it. I think this is the correct way,
>  i wouldn't think we should just count every block queried, even if we'd found an entry in map?
>
> I don't have SPEC, and didn't try it on whole LLVM, but on vanilla test-suite + RawSpeed + darktable,
>  said `IsValueFullyAvailableInBlockNumSpeculationsMax` stat is at max `300`, so i've picked a limit of `600`.
>
> >> 2. We'll need to duplicate `FullyAvailableBlocks` so we pass the identical one into both the old version and the new one. Note that we can't equality-compare them afterwards, because the new code, unlike the old one, in backpropagation, doesn't insert new nodes
> >> 3. The new code doesn't (erroneously) mark previously available successor nodes of an unavailable node unavailable. This technically i guess makes it a non-NFC patch,
> > 
> > Yes I think this shouldn't be marked as NFC.
> > 
> > It would be interesting however, how often the answers diverge (with respect the how the result is used in GVN). For example, it would be interesting to know how often PRE triggers in GVN on some large programs (e.g. MultiSource/SPEC/LLVM bootstrap) and how many differences codegen differences there are between the old and new implementation. Hopefully that would be a very small number, which might lead to a test case for the new behavior.
>
> Let's see if i can catch such a case..


Done. It's rather horrible, but better than nothing i guess.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84181

Files:
  llvm/lib/Transforms/Scalar/GVN.cpp
  llvm/test/Transforms/GVN/loadpre-missed-opportunity.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D84181.280271.patch
Type: text/x-patch
Size: 12773 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200723/772a3007/attachment.bin>