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

[Momchil Velikov via Phabricator via llvm-commits]

chill marked 2 inline comments as done.
chill added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Scalar/GVN.h:246
+  using HoistMap = DenseMap<uint32_t, std::pair<Instruction *, Instruction *>>;
+  HoistMap HoistPairs;
+
----------------
mkazantsev wrote:
> 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?
This map stores pairs of instructions from two blocks having a single common predecessor for the duration of a single top level iteration in `performHoist`.

I will update the comment in that sense.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:3074
+    if (!HasHoistBarrier || isSafeToSpeculativelyExecute(&I))
+      HoistPairs[VN.lookupOrAdd(&I)].first = &I;
+
----------------
mkazantsev wrote:
> 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?
Yes, that's right, we can hoist such instructions.  Should be fine, as we only hoist/merge *pairs* from the two sibling blocks.

In principle, we can hoist (some) following instructions as well, if the two "barrier" instructions are themselves hoisted, but that's sort of expensive to check, so that's a limitation of the algorithm.



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


================
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.
----------------
mkazantsev wrote:
> Why not RPOT?
That is to prevent hoisting an instruction more than once.  Alternatively, we could track how many blocks/instructions up an instruction moves, but that entails an auxiliary data structure.



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

https://reviews.llvm.org/D110817