This is an archive of the discontinued LLVM Phabricator instance.

[BasicAA] Fix AA bug on dynamic allocas and stackrestore
ClosedPublic

Authored by rnk on Dec 20 2018, 4:16 PM.

Details

Summary

BasicAA has special logic for unescaped allocas, which normally applies
equally well to dynamic and static allocas. However, llvm.stackrestore
has the power to end the lifetime of dynamic allocas, without referring
to them directly.

stackrestore is already marked with the most conservative memory
modification attributes, but because the alloca is not escaped, the
normal logic produces incorrect results. I think BasicAA needs a special
case here to teach it about the relationship between dynamic allocas and
stackrestore.

Fixes PR40118

Diff Detail

Repository
rL LLVM

Event Timeline

rnk created this revision.Dec 20 2018, 4:16 PM

I guess stackrestore is special because it's the only way to clobber an unescaped alloca; I think this is fine, in that sense.

That said, I wish IR was defined in a more consistent way; I'm concerned that there are probably other places in LLVM that are making similar bad assumption about dynamic allocas. Since VLAs are essentially banned from most codebases, we get very little test coverage on Linux. I'm particularly concerned about places using PointerMayBeCaptured and similar logic. Is LICM safe? Is DSE safe? Is AAResults::callCapturesBefore safe? Is InstCombiner::foldAllocaCmp safe? Is TailRecursionElimination safe?

rnk added a comment.Dec 20 2018, 5:09 PM

My experience so far with inalloca has been that, no, those passes are not safe. This is only the latest in a long line of bugs in LLVM around handling dynamic allocas. I think conflating static and dynamic allocas was a mistake, and we would've been better served with a separate construct for allocating stack memory for local variables, but at this stage, we'd be better off standardizing on the terminology of "alloca" for standard locals and pushing dynamic allocas into intrinsic instructions.

I'd like to do the same for inalloca, and use tokens to establish a region around the call site. I started a design doc for it, but it looks like I deleted it at some point. :(

efriedma added inline comments.Dec 20 2018, 5:24 PM
llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
1064 ↗(On Diff #179198)

(Please commit this separately.)

llvm/test/Transforms/MemCpyOpt/stackrestore.ll
30 ↗(On Diff #179198)

Please add a corresponding testcase without the stacksave/stackrestore so the test doesn't bitrot.

rnk updated this revision to Diff 179214.Dec 20 2018, 5:48 PM
  • add positive test for dynamic allocas with intervening call
This revision is now accepted and ready to land.Dec 20 2018, 5:51 PM
hfinkel accepted this revision.Dec 20 2018, 7:18 PM
hfinkel added a subscriber: hfinkel.

LGTM

I agree.

This revision was automatically updated to reflect the committed changes.