[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:
> RithikSharma wrote:
> > 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.
> I understand, but the user might pass nullptr (because they don't have PDT) without checking for control flow (by mistake). I would consider this an interface bug to assume that the user checked for control-flow equivalence in the absence of a valid PDT. Instead of //assuming// it, we should make the user be explicit about it.
This makes sense to me, instead of assuming about control flow equivalence the pass would tell us more about it. I will update the patch with the required changes asap.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86850