[PATCH] D100179: [GVN][NFC] Refactor code and add description for GVN object

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Just FYI, recently work on NewGVN has started again, so that's where the focus will (hopefully) be in the future.



================
Comment at: llvm/include/llvm/Transforms/Scalar/GVN.h:112
 
 /// The core GVN pass object.
 ///
----------------
I think this comment would do better in the cpp file, closer to the implementation.


================
Comment at: llvm/include/llvm/Transforms/Scalar/GVN.h:118
+/// \c FunctionAnalysisManager. The method then calls \c runImpl to
+/// start working on the function.
+///
----------------
I don't think this is needed, this is just general pass setup.


================
Comment at: llvm/include/llvm/Transforms/Scalar/GVN.h:124
+/// possible. Within this method, the basic blocks are visited in
+/// a reversed order by calling method \c processBlock, which will
+/// elinimate duplicate PHI nodes, replace equality using information
----------------
reversed order -> reverse post-order (or just RPO)


================
Comment at: llvm/include/llvm/Transforms/Scalar/GVN.h:133
+/// and e) do general value numbering and replacing. The process of load
+/// instruction was supported by method \c AnalyzeLoadAvailability which
+/// analyzes whether a load could be eliminated by using its depedency
----------------
was -> is


================
Comment at: llvm/lib/Transforms/Utils/VNCoercion.cpp:217
-  if (StoreOffset + int64_t(StoreSize) <= LoadOffset)
-    return -1;
-
----------------
Looks right to me, feel free to just commit this separately.


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

https://reviews.llvm.org/D100179