[PATCH] D110817: [GVN] Simple GVN hoist - scalars

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev accepted this revision.
mkazantsev added a comment.
This revision is now accepted and ready to land.

Looks good now, thanks.



================
Comment at: llvm/include/llvm/Transforms/Scalar/GVN.h:367
+  void matchHoistCandidates(BasicBlock *ElseBB);
+  void replaceInstruction(Instruction *, Instruction *);
+  std::pair<bool, bool> hoistPair(BasicBlock *DestBB, BasicBlock *ThenBB,
----------------
nit: please give them some names.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:129
 
+static cl::opt<bool> GVNEnableSimpleGVNHoist("enable-simple-gvnhoist",
+                                             cl::init(true));
----------------
nit: `gvn-hoist`


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:3069
+    if (I.isTerminator())
+      break;
+
----------------
Looks like it also makes sense to skip Phis, just to not bother computing their hashes.


================
Comment at: llvm/test/Transforms/GVN/simple-gvnhoist-scalars.ll:1
+; RUN: opt --passes=gvn -S %s | FileCheck %s
+
----------------
Unless there is a reason to not do so, please use update_test_checks script & precommit the test.


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

https://reviews.llvm.org/D110817