[PATCH] D86850: [LoopRotate][WIP] Check code-motion safety before hoisting

[rithik sharma via Phabricator via llvm-commits]

RithikSharma added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:431
   // TODO remove this limitation.
-  if (!isControlFlowEquivalent(I, InsertPoint, DT, *PDT))
+  if (PDT && !isControlFlowEquivalent(I, InsertPoint, DT, *PDT))
     return reportInvalidCandidate(I, NotControlFlowEquivalent);
----------------
bmahjour wrote:
> This makes it easy to misuse the interface. If someone forgets to pass PDT, we skip checking for control flow equivalence and may return "safe" when it's not the case. I think we should pass a separate boolean to make the user explicitly state whether they care about control-flow equivalence or not.
The basic assumption was that the user will guarantee control flow equivalence as there are not many loop passes that have PDT.


================
Comment at: llvm/lib/Transforms/Utils/LoopRotationUtils.cpp:81
+        IsUtilMode(IsUtilMode), AA(AA) {}
+  bool processLoop(Loop *L, AAResults *AA);
 
----------------
bmahjour wrote:
> AA is a member variable, why does it need to be passed as an argument too?
Thanks, yes we don't require it as an argument, I will update this asap.


================
Comment at: llvm/lib/Transforms/Utils/LoopRotationUtils.cpp:84
 private:
-  bool rotateLoop(Loop *L, bool SimplifiedLatch);
+  bool rotateLoop(Loop *L, bool SimplifiedLatch, AAResults *AA);
   bool simplifyLoopLatch(Loop *L);
----------------
bmahjour wrote:
> why pass AA as argument when it's available as a member?
Same for this, I will update this soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86850