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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for the test case! I think this makes sense and is in the spirit of the original implementation, with some improvements. Some more comments, mostly related to wording inline.



================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:102
+STATISTIC(IsValueFullyAvailableInBlockNumSpeculationsMax,
+          "How many blocks have we speculated as avaliable in "
+          "IsValueFullyAvailableInBlock(), max?");
----------------
For consistency, I would suggest updating the wording to use a similar style to above, e.g. `Number of blocks speculated as available in IsVal..`. Same for the second stat.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:123
+             "into) when deducing if a value is fully avaliable or not in GVN "
+             "(default = unlimited)"));
+
----------------
Is the default not 600?


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:720
+    std::pair<DenseMap<BasicBlock *, AvaliabilityState>::iterator, bool> IV =
+        FullyAvailableBlocks.insert(
+            std::make_pair(CurrBB, AvaliabilityState::SpeculativelyAvailable));
----------------
nit: use try_emplace to skip std::make_pair? (if that compiles)


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:762
+#endif
 
+  // If the block isn't marked as fixpoint yet, do so and enqueue successors
----------------
nit: maybe adjust with something like `as fixpoint yet (the Unavailable and Available states are fixpoints)` to make clear what we mean with fixpoints.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:779
+#endif
+          // *DO* backpropagate further, continue processing worklist.
+          Worklist.append(succ_begin(BB), succ_end(BB));
----------------
As mentioned earlier, it seems odd to me to call propagating to successors as 'back-propagating'. I guess it makes sense if you think about it as back-propagating to the starting node. Might be good to clarify the comment. Might just say `Queue successors for further processing`.


================
Comment at: llvm/test/Transforms/GVN/loadpre-missed-opportunity.ll:2
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -gvn -S | FileCheck %s
 
----------------
also add a line with the limit set so we don't perform the optimization, to check that the limit works?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84181