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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

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.

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



================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:687
+    BasicBlock *RootBB,
+    DenseMap<BasicBlock *, AvaliabilityState> &FixpointBlocks) {
+  SmallVector<BasicBlock *, 32> Worklist;
----------------
Not sure about the name change. It's not clear to me what fixpoint means here. More accurately, its the map of blocks with known states, right? Whatever name is chosen, the comment above needs updating and we should keep the name consistent at the call sites.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:753
+          Worklist.append(succ_begin(BB), succ_end(BB));
+          return; // *DO* backpropagate further, continue processing worklist.
+        }
----------------
The place of the comment next to the return seems a bit strange to me. IMO it would make more sense to have a comment above Worklist.append, stating we queue the successors to continue back-propagating.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:759
+    // Okay, we have encountered an "unavailable" block.
+    // Backpropagate that information to the each non-fixpoint successor.
+    Worklist.clear();
----------------
Again, it is not entirely clear what "non-fixpoint" refers to here. It only means blocks marked as SpeculativelyAvailable, right? Also, back-propagating seems a bit counter-intuitive here at a first glance, if you think of the CFG as a directed graph (edges directed from BB to its successors).

Saying something like 

```
// Okay, we have encountered an "unavailable" block.
// Mark SpeculativelAvailable blocks reachable from UnavailableBB as unavailable as well. Paths are terminated when they reach blocks no in Fixpoints or they are not marked as SpeculativelAvailable.

```


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:779
 
-    // Mark as unavailable.
-    EntryVal = AvaliabilityState::Unavaliable;
-
-    BBWorklist.append(succ_begin(Entry), succ_end(Entry));
-  } while (!BBWorklist.empty());
-
-  return false;
+  return !UnavailableBB.hasValue();
 }
----------------
nit: could this just `return !UnabilableBB`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84181