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

Thu Jul 23 00:31:15 PDT 2020

lebedev.ri added a reviewer: jdoerfert.
lebedev.ri added a subscriber: lattner.
lebedev.ri added a comment.

In D84181#2168668 <https://reviews.llvm.org/D84181#2168668>, @mkazantsev wrote:

> Though I don't see obvious problems in the new algorithm, I cannot wrap my mind around the old one

Yeah, i had that feeling myself :/ That's why i've put @lattner as reviewer,
since it was him who seems to have wrote/modified it in rL60408 <https://reviews.llvm.org/rL60408>/rL60588 <https://reviews.llvm.org/rL60588> ~12 years ago.

> and can't say if they are doing the same thing.

Any particular part that is causing trouble?
The code essentially consists of two separate parts, "maze solving" and backpropagation.

I think, backpropagation is obviously identical at least?
We start at the first block we've found to be an unavailable.
We need to mark each successor block, that isn't known to be available, as unavailable.
Also, to not pay for what we don't use, we should only mark the blocks that we care about,
i.e. those that are already present in the FullyAvailableBlocks map.
So we put said block into worklist, and do pretty typical worklist dance,
take block from worklist, and query (with insert!) it's availability status in a map.
WARNING that in the old version, if the block wasn't in a map already, this adds it as unavailable!
Then, if we have found out that it was unavailable, we just go to the next entry in worklist.
If it wasn't unavailable, we mark it as such
(WARNING: in old version, it would also mark available blocks as unavailable, which is obviously bogus,
while we should only do that for speculatively available blocks),
and proceed to further backpropagate that information to it's successors (by putting them into worklist).

The "maze solving" seems pretty intuitive in hindsight, too:
We start at some block, we want to know if some value is available in that block.
So we see if the map already contains an entry for the block, then we know our answer,
it is either explicitly marked as unavailable, or it's available/speculatively available.
Otherwise, as a perf optimization, we think it will be available, so that added an entry to the map,
marking the block as speculatively available.
Now, we need to check if the value can live-in from each(!) predecessor.
The value can't live-in if there are no predecessors and the block is unavailable.
If there are predecessors, we need to visit each one and do the all over again.

As soon as we find out that the block is unavailable, we have a problem,
we may have marked blocks as speculatively available along the way to the unavailable block.
It is important to notice that we don't deviate from our exploration path,
so all the blocks we have just marked as speculatively available are successors of this block.

Otherwise, if we never encounter an unavailable block, then we're all good.
Since we would have retrospectively fixed all the blocks we have set as speculatively available
as unavailable, from now on, all these speculatively available blocks are available.

All this logic is verified with asserts in the end.

> 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
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,

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84181