Page MenuHomePhabricator

[NewPM] Only invalidate modified functions' analyses in CGSCC passes
ClosedPublic

Authored by aeubanks on Tue, Apr 20, 6:39 PM.

Details

Summary

Previously, any change in any function in an SCC would cause all
analyses for all functions in the SCC to be invalidated. With this
change, we now manually invalidate analyses for functions we modify,
then let the pass manager know that all function analyses should be
preserved.

So far this only touches the inliner, argpromotion, funcattrs, and
updateCGAndAnalysisManager(), since they are the most used.

Slight compile time improvements:
http://llvm-compile-time-tracker.com/compare.php?from=326da4adcb8def2abdd530299d87ce951c0edec9&to=8942c7669f330082ef159f3c6c57c3c28484f4be&stat=instructions

Diff Detail

Event Timeline

aeubanks created this revision.Tue, Apr 20, 6:39 PM
aeubanks requested review of this revision.Tue, Apr 20, 6:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Apr 20, 6:39 PM

I'm happy to split this up, I'd just like to know if this is reasonable.

check-llvm with expensive checks passes.

mtrofin accepted this revision.Tue, Apr 20, 7:47 PM

lgtm

This revision is now accepted and ready to land.Tue, Apr 20, 7:47 PM
ychen added a subscriber: ychen.Tue, Apr 20, 10:56 PM

I think we should be careful about letting passes invalidate analyses directly. IIUC the only interface for propagating invalidation from passes is PreservedAnalyses. Is there any way to enhance PreservedAnalyses to achieve the same effect?

It seems awkward to enhance PreservedAnalyses to only apply to specific functions when this patch is explicitly doing exactly what we want. If you have a clean way of modeling this inside PreservedAnalyses I'm happy to go with that, but so far I think this is the cleanest way.
Passes were already directly managing the analysis managers, e.g. the inliner and argpromo would remove entries for deleted functions. And updateCGAndAnalysisManager() would also handle analysis manager updates.

ychen added a comment.Wed, Apr 21, 4:57 PM

It seems awkward to enhance PreservedAnalyses to only apply to specific functions when this patch is explicitly doing exactly what we want. If you have a clean way of modeling this inside PreservedAnalyses I'm happy to go with that, but so far I think this is the cleanest way.

I was thinking something like PreservedAnalyses::abandon(IRUnit, AnalysisID) which only used to selectively invalidate inner IR units, but I've convinced myself that it may not worth it. Invalidating inner analyses through proxy should work well already.

Passes were already directly managing the analysis managers, e.g. the inliner and argpromo would remove entries for deleted functions. And updateCGAndAnalysisManager() would also handle analysis manager updates.

I would argue that updateCGAndAnalysisManager() is kinda special (for coping with the CGSCCPM's complexity). I think the one in argpromo is not necessary since it already returns PreservedAnalyses::none(). inliner yes.

llvm/lib/Transforms/IPO/FunctionAttrs.cpp
1637

Is the function CFG preserved?

aeubanks added inline comments.Wed, Apr 21, 9:34 PM
llvm/lib/Transforms/IPO/FunctionAttrs.cpp
1637

Yes, will update.
Doing this causes globalaa-retained.ll to fail, which I believe is fixed by https://reviews.llvm.org/D101017

aeubanks updated this revision to Diff 339782.Thu, Apr 22, 2:19 PM

add test
use CFGAnalyses
uncovered another place in updateCGAndAnalysisManager() to change

depends on D101017

Herald added a project: Restricted Project. · View Herald TranscriptThu, Apr 22, 2:19 PM
aeubanks edited the summary of this revision. (Show Details)Thu, Apr 22, 2:20 PM
This revision was landed with ongoing or failed builds.Mon, May 3, 5:22 PM
This revision was automatically updated to reflect the committed changes.
nikic added a comment.Tue, May 4, 12:36 AM

An unfortunate side-effect of this change is that NewPM uses even more memory. tramp3d-v4 is up 20% in max-rss (https://llvm-compile-time-tracker.com/compare.php?from=4ef1f90e4d564b872e3598ccef45adb740eb0f0d&to=d14d84af2f5ebb8ae2188ce6884a29a586dc0a40&stat=max-rss)

An unfortunate side-effect of this change is that NewPM uses even more memory. tramp3d-v4 is up 20% in max-rss (https://llvm-compile-time-tracker.com/compare.php?from=4ef1f90e4d564b872e3598ccef45adb740eb0f0d&to=d14d84af2f5ebb8ae2188ce6884a29a586dc0a40&stat=max-rss)

Hmm that is a concern. I'm not sure how we want to balance memory vs compile times. Any thoughts?

An unfortunate side-effect of this change is that NewPM uses even more memory. tramp3d-v4 is up 20% in max-rss (https://llvm-compile-time-tracker.com/compare.php?from=4ef1f90e4d564b872e3598ccef45adb740eb0f0d&to=d14d84af2f5ebb8ae2188ce6884a29a586dc0a40&stat=max-rss)

Hmm that is a concern. I'm not sure how we want to balance memory vs compile times. Any thoughts?

(Drive-by thought) - maybe this is because a bunch of analyses that are needed during function simplification aren't needed anymore after. We could quickly check by adding a pass at the end of all function simplification, module-wide, that clears all fct analyses, and see if the memory overhead is still there?

If that's the case, we could identify what's not needed after function simplification and scope it somehow in a less hacky way than my above suggestion.