[PATCH] D67941: Invalidate assumption cache before outlining.

[Vedant Kumar via Phabricator via llvm-commits]

vsk accepted this revision.
vsk added a comment.
This revision is now accepted and ready to land.

Thanks, lgtm with some minor test updates.



================
Comment at: llvm/lib/Transforms/Utils/CodeExtractor.cpp:1377
+        if (match(&I, m_Intrinsic<Intrinsic::assume>()))
+          AC->unregisterAssumption(cast<CallInst>(&I));
+  }
----------------
hiraditya wrote:
> 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.
Thanks for explaining, it makes sense to move the un-registration before splitting occurs.


================
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:
> > 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.
> I'm not sure how to make `-implicit-check-not` work when llvm.assume is present in the function. I can use some help, can you share an example? For now I used CHECK to make sure llvm.assume isn't present in the original function.
Hm, my theory is that the `-implicit-check-not` is probably causing a test failure because it's matching the //declaration// of `@llvm.assume`.

The CHECK-NOT you've added in the original function's `codeRepl` block probably makes the `implicit-check-not` redundant, though.


================
Comment at: llvm/test/Transforms/HotColdSplit/assumption-cache-invalidation.ll:15
+; CHECK: }
+; CHECK: @llvm.assume
+; CHECK: @f.cold.1(i64 %0)
----------------
Should this be "declare {{.*}}@llvm.assume"?


================
Comment at: llvm/test/Transforms/HotColdSplit/assumption-cache-invalidation.ll:16
+; CHECK: @llvm.assume
+; CHECK: @f.cold.1(i64 %0)
+; CHECK-LABEL: newFuncRoot:
----------------
Should this be "define {{.*}}@f.cold.1("?


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

https://reviews.llvm.org/D67941