Page MenuHomePhabricator

[CodeExtractor] Update function's assumption cache after extracting blocks from it
ClosedPublic

Authored by sdmitriev on Thu, Jan 24, 6:27 PM.

Details

Summary

Assumption cache's self-updating mechanism does not correctly handle the case when blocks are extracted from the function by the CodeExtractor. As a result function's assumption cache may have stale references to the llvm.assume calls that were moved to the outlined function. This patch fixes this problem by removing extracted llvm.assume calls from the function’s assumption cache.

Diff Detail

Repository
rL LLVM

Event Timeline

sdmitriev created this revision.Thu, Jan 24, 6:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Feb 6, 1:23 PM
vsk added a comment.Wed, Feb 6, 2:03 PM

@sdmitriev thanks for the patch!

lib/Transforms/IPO/HotColdSplitting.cpp
202 ↗(On Diff #183463)

The splitting pass does not explicitly preserve AssumptionCacheTracker. Why does this not invalidate AssumptionCacheTracker correctly? As the splitting pass does not "really" need AssumptionCacheTracker, it seems preferable to just not require it here.

sdmitriev added inline comments.Wed, Feb 6, 2:52 PM
lib/Transforms/IPO/HotColdSplitting.cpp
202 ↗(On Diff #183463)

AssumptionCacheTracker is implemented as immutable pass, so, as I understand, it cannot be invalidated.

vsk accepted this revision.Wed, Feb 6, 3:30 PM
vsk added reviewers: davidxl, sanjoy.

LGTM, but, please wait for a +1 from someone more familiar with AssumptionCache before committing.

lib/Transforms/IPO/HotColdSplitting.cpp
202 ↗(On Diff #183463)

I see, thanks for explaining. It looks like it's well-understood that AssumptionCache may need to be invalidated (there's already a clear() method). As most passes do not invalidate it, making AssumptionCacheTracker an ImmutablePass might be the right tradeoff. @hfinkel does that sound right?

lib/Transforms/IPO/PartialInlining.cpp
184 ↗(On Diff #183463)

nit, as this seems unrelated to the main change, perhaps it could be a followup?

This revision is now accepted and ready to land.Wed, Feb 6, 3:30 PM
sdmitriev marked an inline comment as done.Wed, Feb 6, 3:45 PM
sdmitriev added inline comments.
lib/Transforms/IPO/PartialInlining.cpp
184 ↗(On Diff #183463)

Right, this is not related to the main change. I just noticed that Region is passed by value which is not very efficient. I will remove this change from this patch.

sdmitriev updated this revision to Diff 185662.Wed, Feb 6, 4:15 PM

Removed unrelated change (lib/Transforms/IPO/PartialInlining.cpp, line 184) from the patch.

hfinkel added inline comments.Wed, Feb 6, 5:34 PM
lib/Transforms/IPO/HotColdSplitting.cpp
202 ↗(On Diff #183463)

The original idea was that the only passes that added new assumptions needed to explicitly update the cache, and for everything else it should be self-updating (and this made sense under the assumption that adding assumptions is rare).

hfinkel added inline comments.Wed, Feb 6, 7:02 PM
lib/Transforms/Utils/CodeExtractor.cpp
1422 ↗(On Diff #185662)

Can we clear here less than everything?

sdmitriev updated this revision to Diff 185839.Thu, Feb 7, 12:01 PM
sdmitriev retitled this revision from [CodeExtractor] Clear function's assumption cache after extracting blocks from it to [CodeExtractor] Update function's assumption cache after extracting blocks from it.
sdmitriev edited the summary of this revision. (Show Details)

Revised changes to remove extracted llvm.assume calls from the old function's assumption cache instead of clearing it.

sdmitriev marked an inline comment as done.Thu, Feb 7, 12:05 PM
sdmitriev added inline comments.
lib/Transforms/Utils/CodeExtractor.cpp
1422 ↗(On Diff #185662)

I have updated changes to remove extracted llvm.assume calls from the old function's assumption cache instead of clearing it.

hfinkel added inline comments.Thu, Feb 7, 12:37 PM
lib/Transforms/Utils/CodeExtractor.cpp
1422 ↗(On Diff #185662)

But then they'll be missing and it won't rescan? Why don't we add an interface like movedToNewFunction(CallInst *CI), and let the cache actually update itself properly?

sdmitriev marked an inline comment as done.Thu, Feb 7, 2:03 PM
sdmitriev added inline comments.
lib/Transforms/Utils/CodeExtractor.cpp
1422 ↗(On Diff #185662)

But then they'll be missing and it won't rescan?

Rescanning for the old function (where the blocks are extracted from) is not really needed since I am removing all llvm.assume calls that were moved to a new function from its cache. Therefore its cache is supposed to be up-to-date.

And the new function (that is created by the extractor) does not yet have assumption cache created at this point, so it will be scanned on demand when its assumption cache is requested.

Why don't we add an interface like movedToNewFunction(CallInst *CI), and let the cache actually update itself properly?

OK, let me check if I understand your suggestion correctly. You propose to add such interface to the AssumptionCache class, and it is supposed to remove CI’s assumptions from ‘this’ and add CI to the new function’s cache if it exists (otherwise new function will be rescanned anyway). Is that right?

I guess that would require adding a reference to the AssumptionCacheTracker to the AssumptionCache class since it does not currently have access to the other function’s cache. Would that be OK?

hfinkel accepted this revision.Thu, Feb 7, 8:14 PM
hfinkel added inline comments.
lib/Transforms/Utils/CodeExtractor.cpp
1422 ↗(On Diff #185662)

Okay, that makes sense. This LGTM.

This revision was automatically updated to reflect the committed changes.