This is an archive of the discontinued LLVM Phabricator instance.

[stack-safety] Inter-Procedural Analysis implementation
ClosedPublic

Authored by vitalybuka on Nov 14 2018, 1:53 PM.

Details

Summary

IPA is implemented as module pass which produce map from Function or Alias to
StackSafetyInfo for a single function.

From prototype by Evgenii Stepanov and Vlad Tsyrklevich.

Diff Detail

Repository
rL LLVM

Event Timeline

vitalybuka created this revision.Nov 14 2018, 1:53 PM

cleanup includes

mgrang added inline comments.Nov 14 2018, 2:37 PM
llvm/lib/Analysis/StackSafetyAnalysis.cpp
494 ↗(On Diff #174098)

Please use the range-based llvm::sort(Callees) instead of std::sort.

See https://llvm.org/docs/CodingStandards.html#beware-of-non-deterministic-sorting-order-of-equal-elements

vitalybuka marked an inline comment as done.

llvm::sort

The local tests that check for use 'empty-set, $CALL_INFO' for the local and 'full-set, $CALL_INFO' for the global analysis is sort of confusing. It seems like the use is safe in local analysis (empty-set means no uses!) but unsafe in the global, when really the situation is actually local analysis has 'no local uses but used in function calls' and the global analysis is just 'it's safe/unsafe (function call info is integrated into the previous distintion)'. Maybe we should clear all of the UseInfo::Calls fields at the end of StackSafetyDataFlowAnalysis?

llvm/lib/Analysis/StackSafetyAnalysis.cpp
147 ↗(On Diff #174098)

s/'function' summary/FunctionInfo/

599 ↗(On Diff #174098)

Do we need this comment here and in StackSafetyDataFlowAnalysis?

634 ↗(On Diff #174098)

Do we need this comment here and in StackSafetyDataFlowAnalysis?

The local tests that check for use 'empty-set, $CALL_INFO' for the local and 'full-set, $CALL_INFO' for the global analysis is sort of confusing. It seems like the use is safe in local analysis (empty-set means no uses!) but unsafe in the global, when really the situation is actually local analysis has 'no local uses but used in function calls' and the global analysis is just 'it's safe/unsafe (function call info is integrated into the previous distintion)'. Maybe we should clear all of the UseInfo::Calls fields at the end of StackSafetyDataFlowAnalysis?

That's unfortunate, as now we can keep checks for local and global passes mostly the same and highlight the difference with branched LOCAL-NEXT/GLOBAL-NEXT

That's unfortunate, as now we can keep checks for local and global passes mostly the same and highlight the difference with branched LOCAL-NEXT/GLOBAL-NEXT

That's a good point, I didn't consider that it would break that pattern. Though looking at the tests now it seems that the current tests don't have cases where this would happen? Presumably most tests where an alloca has a use in a function call it would result in a local/global difference due to the IPA being able to refine the use range.

That's unfortunate, as now we can keep checks for local and global passes mostly the same and highlight the difference with branched LOCAL-NEXT/GLOBAL-NEXT

That's a good point, I didn't consider that it would break that pattern. Though looking at the tests now it seems that the current tests don't have cases where this would happen? Presumably most tests where an alloca has a use in a function call it would result in a local/global difference due to the IPA being able to refine the use range.

Can we reconsider this later. I've added TODO. Probably these are implementation details which are unneeded.

vitalybuka marked 3 inline comments as done.

Can we reconsider this later. I've added TODO. Probably these are implementation details which are unneeded.

I'm OK coming back to it after the bulk of the changes have landed.

eugenis added inline comments.Nov 20 2018, 4:54 PM
llvm/lib/Analysis/StackSafetyAnalysis.cpp
380 ↗(On Diff #174283)

UpdateCount could be moved to FunctionInfo to save a hash map lookup.

421 ↗(On Diff #174283)

Use DataLayout::getPointerSizeInBits instead of 64.

514 ↗(On Diff #174283)

This makes verifyFixedPoint unused in no-assertions build. Move it under LLVM_DEBUG, too.

vitalybuka marked 2 inline comments as done.

@eugenis comments

llvm/lib/Analysis/StackSafetyAnalysis.cpp
514 ↗(On Diff #174283)

I'd rather keep unused function than introducing "#ifndef NDEBUG"

eugenis accepted this revision.Nov 21 2018, 3:38 PM

LGTM with nit

llvm/lib/Analysis/StackSafetyAnalysis.cpp
187 ↗(On Diff #174986)

getPointerSizeInBits implementation is not trivial, consider caching the result in StackSafetyLocalAnalysis

This revision is now accepted and ready to land.Nov 21 2018, 3:38 PM
vitalybuka marked an inline comment as done.

update

vlad.tsyrklevich added inline comments.
llvm/lib/Analysis/StackSafetyAnalysis.cpp
428 ↗(On Diff #175003)

s/ and annotate allocas//

This revision was automatically updated to reflect the committed changes.
jfb reopened this revision.Nov 26 2018, 3:51 PM
jfb added a subscriber: jfb.

Please fix the build break introduced.

llvm/trunk/lib/Analysis/StackSafetyAnalysis.cpp
466

This doesn't build:

../lib/Analysis/StackSafetyAnalysis.cpp:465:16: error: no member named 'Range' in '(anonymous namespace)::PassAsArgInfo'
    assert(!CS.Range.isEmptySet() &&
            ~~ ^

I commented it out in r347616. Please fix.

This revision is now accepted and ready to land.Nov 26 2018, 3:51 PM
vitalybuka closed this revision.Nov 26 2018, 4:49 PM

Thanks, I'll fix the assert.