Page MenuHomePhabricator

Invalidate assumption cache before outlining.
ClosedPublic

Authored by hiraditya on Sep 23 2019, 4:26 PM.

Details

Summary

Assumption cache pointers become stale after function splitting and dependencies from old function can't be removed as such.
This patch:

  1. un-registers the assumption cache before basic blocks are outlined
  2. verifies the AssumptionCache of the original function to make sure no stale entries remain
  3. Fixed a bug in assumption cache where the AssumeHandle wasn't removing an assume instrinsic while unregistering an Assumption

See D67936 for the quickfix to this bug.

Diff Detail

Event Timeline

hiraditya created this revision.Sep 23 2019, 4:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2019, 4:26 PM
hiraditya updated this revision to Diff 221434.Sep 23 2019, 4:29 PM
hiraditya added reviewers: vsk, tejohnson.

Please post patches with full context (the full source file should be here). I can't see where moveCodeToFunction is called.

hiraditya updated this revision to Diff 221455.Sep 23 2019, 7:26 PM
vsk added a comment.Sep 24 2019, 1:52 PM

Is there a test case for the assertion failure? (I'm assuming that the test attached to D67936 does _not_, is that right?)

The test case in D67936 does assert, I mean without the fix from that diff.

vsk added a comment.Sep 24 2019, 2:27 PM

Ah, could you add a test for this that asserts with D67936 applied?

With D67936, it wont assert anymore (with hot-cold-split). That was a quick fix but the real bug is in the CodeExtractor. let me add a revert of that patch in here as well. Because with this patch we wont need D67936.

hiraditya updated this revision to Diff 221729.Sep 25 2019, 5:38 AM

Reverting r372667

vsk added inline comments.Sep 25 2019, 3:00 PM
llvm/lib/Transforms/Utils/CodeExtractor.cpp
1377

Is the findAffectedValues() operation inside of AssumptionCache not finding values because the llvm.assume is moved to another function? I.e. the search stops at an Argument? I think adding a unit test to CodeExtractorTest.cpp would really help.

hiraditya marked an inline comment as done.Sep 25 2019, 3:25 PM
hiraditya added inline comments.
llvm/lib/Transforms/Utils/CodeExtractor.cpp
1377

Basically the assertion: ValueTracking.cpp: 594 assert(I->getParent()->getParent() == Q.CxtI->getParent()->getParent() &&... triggers because one of the values is not unregistered even after it was moved to a new function. The value which are moved to new function get successfully unregistered without this patch but the values which remained in the original function didnt. This is because pointers to values got updated and Assumption cache had stale pointers.

Let me try to add a test case to CodeExtractorTest.cpp, seems slightly tricky from the first glance.

hiraditya edited the summary of this revision. (Show Details)

Added AssumptionCache verifier to verify the original function.

@vsk I tried adding test to CodeExtractorTest (D68095), for some reason the verifier didn't fail. I may be missing something and can use some help.
We can follow up on the test in the new diff if that is reasonable.

Is there any feedback for improvement, I'd like to push this if reasonable.

fhahn added inline comments.Sep 29 2019, 2:41 PM
llvm/lib/Transforms/Utils/CodeExtractor.cpp
1569

Can we just use AC->verifyAnalysis()?

hiraditya marked an inline comment as done.Sep 29 2019, 3:01 PM
hiraditya added inline comments.
llvm/lib/Transforms/Utils/CodeExtractor.cpp
1569

It seems, verifyAnalysis only checks if the AssumptionCache has the entry for a llvm.assume instruction or not.

AssumptionCache.cpp:268
        if (match(&II, m_Intrinsic<Intrinsic::assume>()) &&
            !AssumptionSet.count(cast<CallInst>(&II)))
          report_fatal_error("Assumption in scanned function not in cache");
hiraditya added a comment.EditedSep 29 2019, 7:14 PM

@vsk Fixed the unittest in: D68095, it triggers the failure when AssumptionCache is stale.

vsk added a comment.Sep 30 2019, 1:03 PM

I see, this looks good to me with a small test update.

llvm/test/Transforms/HotColdSplit/assumption-cache-invalidation.ll
3

Pass '-implicit-check-not=llvm.assume' to FileCheck, so this tests that the assumption in '@f' was removed?

hiraditya updated this revision to Diff 222501.Sep 30 2019, 2:50 PM

added flag to the testcase.

hiraditya marked an inline comment as done.Sep 30 2019, 2:51 PM
hiraditya updated this revision to Diff 222599.Oct 1 2019, 6:07 AM
hiraditya updated this revision to Diff 222601.Oct 1 2019, 6:56 AM
hiraditya edited the summary of this revision. (Show Details)

Fixed a bug in assumption cache where the AssumeVH wasn't getting cleared.

hiraditya marked an inline comment as done.Oct 1 2019, 6:57 AM
hiraditya added inline comments.
llvm/test/Transforms/HotColdSplit/assumption-cache-invalidation.ll
3

the llvm.assume will be there in the new function after outlining.

vsk added inline comments.Oct 1 2019, 1:14 PM
llvm/lib/Transforms/Utils/CodeExtractor.cpp
1377

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.

1568

The if (AC) check needs to be wrapped in LLVM_DEBUG, otherwise the function will end without returning anything in Release mode.

1577

I->getFunction(), please.

llvm/test/Transforms/HotColdSplit/assumption-cache-invalidation.ll
3

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.

hiraditya marked 3 inline comments as done.Oct 1 2019, 1:41 PM
hiraditya added inline comments.
llvm/lib/Transforms/Utils/CodeExtractor.cpp
1377

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.

1568

It shouldn't because of ';', but i see the point.

1577

sure.

hiraditya updated this revision to Diff 222712.Oct 1 2019, 2:33 PM
hiraditya marked an inline comment as done.
hiraditya added inline comments.
llvm/test/Transforms/HotColdSplit/assumption-cache-invalidation.ll
3

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.

hiraditya marked an inline comment as done.Oct 1 2019, 2:36 PM
vsk accepted this revision.Oct 2 2019, 2:08 PM

Thanks, lgtm with some minor test updates.

llvm/lib/Transforms/Utils/CodeExtractor.cpp
1377

Thanks for explaining, it makes sense to move the un-registration before splitting occurs.

llvm/test/Transforms/HotColdSplit/assumption-cache-invalidation.ll
3

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.

15

Should this be "declare {{.*}}@llvm.assume"?

16

Should this be "define {{.*}}@f.cold.1("?

This revision is now accepted and ready to land.Oct 2 2019, 2:08 PM
hiraditya marked 2 inline comments as done.Oct 2 2019, 2:13 PM
hiraditya added inline comments.
llvm/test/Transforms/HotColdSplit/assumption-cache-invalidation.ll
3

Hm, my theory is that the -implicit-check-not is probably causing a test failure because it's matching the declaration of @llvm.assume.

possible.

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

Thanks.

15

I'll make these changes before committing.