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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Scalar/GVN.h:246
+  using HoistMap = DenseMap<uint32_t, std::pair<Instruction *, Instruction *>>;
+  HoistMap HoistPairs;
+
----------------
Not entirely clear from description which pairs does this store. What if we have N equivalent instructions in different paths, none dominating other? How many (and which) pairs is it going to store?


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:3074
+    if (!HasHoistBarrier || isSafeToSpeculativelyExecute(&I))
+      HoistPairs[VN.lookupOrAdd(&I)].first = &I;
+
----------------
What worries me is that when we meet a non-guaranteed-to-transfer-execution instruction, we add it to HoistPairs before we actually raise the flag. Does it mean we can sometimes hoist such instructions?


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:3077
+    if ((HasHoistBarrier |= isHoistBarrier(I)))
+      LLVM_DEBUG(dbgs() << "Simple GVNHoist: reached hoist barrier" << I
+                        << '\n';);
----------------
nit: space after "barrier".


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:3182
+  bool Change = false;
+  for (BasicBlock *BB : depth_first(&F.getEntryBlock())) {
+    // Check we have a block of the desired shape.
----------------
Why not RPOT?


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

https://reviews.llvm.org/D110817