[PATCH] D57215: [CodeExtractor] Update function's assumption cache after extracting blocks from it

[Hal Finkel via Phabricator via llvm-commits]

hfinkel accepted this revision.
hfinkel added inline comments.


================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:1422
+  if (AC)
+    AC->clear();
+
----------------
sdmitriev wrote:
> hfinkel wrote:
> > sdmitriev wrote:
> > > hfinkel wrote:
> > > > Can we clear here less than everything?
> > > I have updated changes to remove extracted llvm.assume calls from the old function's assumption cache instead of clearing it.
> > 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?
> > 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?
> 
Okay, that makes sense. This LGTM.


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

https://reviews.llvm.org/D57215