[PATCH] D67941: Invalidate assumption cache before outlining.

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CodeExtractor.cpp:1377
+        if (match(&I, m_Intrinsic<Intrinsic::assume>()))
+          AC->unregisterAssumption(cast<CallInst>(&I));
+  }
----------------
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.


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


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


================
Comment at: llvm/test/Transforms/HotColdSplit/assumption-cache-invalidation.ll:2
+; REQUIRES: asserts
+; RUN: opt -S -instsimplify -hotcoldsplit -debug < %s 2>&1 | FileCheck %s
 ; RUN: opt -instcombine -hotcoldsplit -instsimplify %s -o /dev/null
----------------
hiraditya wrote:
> vsk wrote:
> > Pass '-implicit-check-not=llvm.assume' to FileCheck, so this tests that the assumption in '@f' was removed?
> the llvm.assume will be there in the new function after outlining.
Yes, I see that you've added a check for the llvm.assume in the new (split) function. Please add the -implicit-check-not as well, to test that no llvm.assume is left in the original function.


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

https://reviews.llvm.org/D67941