[PATCH] D109760: [GVN] Simple GVN hoist

[JunMa via Phabricator via llvm-commits]

junparser added a comment.

Thanks for your patch！



================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:2494
+    bool RunAgain = true;
+    while (RunAgain) {
+      bool HoistedAny;
----------------
It is better to add parameter like gvn-hoist-max-chain-length in gvnhoist here based on running data from spec.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:2842
+  return I.isVolatile() || I.isAtomic() ||
+         !isGuaranteedToTransferExecutionToSuccessor(&I);
+}
----------------
can you give an example that when I can not transfer execution, how do subsequent instructions in BB to be hoisted?  IMHO, collectHoistCandidates should stop here.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:2860
+    // appearing as operands in hoisted instructions).
+    if (isa<GetElementPtrInst>(&I))
+      continue;
----------------
Is there any reason not to handle GEPs?


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:3034
+        // Hoisting these creates opportunities for more hoisting.
+        if (P.first->mayReadOrWriteMemory() || isHoistBarrier(*P.first))
+          RunAgain = true;
----------------
ditto


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

https://reviews.llvm.org/D109760