This is an archive of the discontinued LLVM Phabricator instance.

[stack-safety] Allow to determine safe accesses.
ClosedPublic

Authored by fmayer on Sep 9 2021, 5:27 AM.

Diff Detail

Event Timeline

fmayer created this revision.Sep 9 2021, 5:27 AM
fmayer requested review of this revision.Sep 9 2021, 5:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2021, 5:27 AM
fmayer updated this revision to Diff 371580.Sep 9 2021, 6:48 AM

add more tests

vitalybuka accepted this revision.Sep 9 2021, 11:57 AM

LGTM if you agree with my changes

llvm/include/llvm/Analysis/StackSafetyAnalysis.h
81

"&I" for consistency

llvm/lib/Analysis/StackSafetyAnalysis.cpp
137

you missed my comment about auto vs tie.

this will work:

std::map<const Instruction *, /*remove const*/ ConstantRange> Accesses;
...
void addRange(const Instruction *I, const ConstantRange &R) {
    auto Ins = Accesses.emplace(I, R);
    if (!Ins.second)
      Ins.first->second = unionNoWrap(Ins.first->second, R);
    updateRange(R);
  }
232–233

for consistency with line above
SmallPtrSet<const Instruction *, 8> SafeAccesses;

This revision is now accepted and ready to land.Sep 9 2021, 11:57 AM
vitalybuka added inline comments.Sep 9 2021, 12:04 PM
llvm/lib/Analysis/StackSafetyAnalysis.cpp
361

I see why you need "break" instead of "return" in these cases.
However I tried to revert this part and no tests failed.

Could you please to add a test-case sensitive to this part of the patch. Please cover at least few of these breaks.

LGTM with Vitaly's comment about test coverage

llvm/include/llvm/Analysis/StackSafetyAnalysis.h
81

I realized that this header does not define what "safe" means anywhere. Especially important for this method. Could you clarify that this means the access is proven to be inbounds of a live alloca?

llvm/lib/Analysis/StackSafetyAnalysis.cpp
820

"UnsafeAccesses" ?
"AccessIsUnsafe"?

vitalybuka added inline comments.Sep 9 2021, 3:03 PM
llvm/lib/Analysis/StackSafetyAnalysis.cpp
820

@eugenis suggestion LGTM

fmayer updated this revision to Diff 371921.Sep 10 2021, 7:37 AM
fmayer marked 6 inline comments as done.

add more tests

fmayer updated this revision to Diff 371923.Sep 10 2021, 7:51 AM
fmayer marked an inline comment as done.

simplify test

fmayer added inline comments.Sep 10 2021, 7:53 AM
llvm/lib/Analysis/StackSafetyAnalysis.cpp
361

Added tests covering most of the return -> break cases.

vitalybuka accepted this revision.Sep 10 2021, 11:05 AM

LGTM

llvm/lib/Analysis/StackSafetyAnalysis.cpp
820–837

this one is not done

fmayer updated this revision to Diff 371971.Sep 10 2021, 11:20 AM
fmayer marked an inline comment as done.

fix name

fmayer updated this revision to Diff 371972.Sep 10 2021, 11:21 AM

name

llvm/lib/Analysis/StackSafetyAnalysis.cpp
820–837

Sorry, I was sure I did this, but must have messed up the upload somehow.

This revision was landed with ongoing or failed builds.Sep 10 2021, 11:24 AM
This revision was automatically updated to reflect the committed changes.