[PATCH] D67941: Invalidate assumption cache before outlining.

[Aditya Kumar via Phabricator via llvm-commits]

hiraditya marked 3 inline comments as done.
hiraditya added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CodeExtractor.cpp:1377
+        if (match(&I, m_Intrinsic<Intrinsic::assume>()))
+          AC->unregisterAssumption(cast<CallInst>(&I));
+  }
----------------
vsk wrote:
> Could you explain the reason for the movement of logic that unregisters assumptions from `moveCodeToFunction` to here, before the new function is created? I don't understand why it's necessary.
I think we should fix assumption cache before moving the blocks to new function. Once we move the block to new function the instructions technically belong to the new function and assumption cache still treats them as if belonging to the old function. Even if clearing the AC after splitting works, it doesn't appear to be logically the right place to put it. I'm happy to put this where it was as I dont have a preference.


================
Comment at: llvm/lib/Transforms/Utils/CodeExtractor.cpp:1568
              report_fatal_error("verification of oldFunction failed!"));
+  if (AC)
+    LLVM_DEBUG(if (verifyAssumptionCache(*oldFunction, AC))
----------------
vsk wrote:
> The `if (AC)` check needs to be wrapped in `LLVM_DEBUG`, otherwise the function will end without returning anything in Release mode.
It shouldn't because of ';', but i see the point.


================
Comment at: llvm/lib/Transforms/Utils/CodeExtractor.cpp:1578
+    CallInst *I = cast<CallInst>(AssumeVH);
+    if (I->getParent()->getParent() != &F)
+      return true;
----------------
vsk wrote:
> `I->getFunction()`, please.
sure.


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

https://reviews.llvm.org/D67941