This is an archive of the discontinued LLVM Phabricator instance.

[FIX] Forces shrink wrapping to consider any memory access as aliasing with the stack
ClosedPublic

Authored by dnsampaio on Jun 11 2019, 10:46 AM.

Details

Summary

Relate bug: https://bugs.llvm.org/show_bug.cgi?id=37472

The shrink wrapping pass prematurally restores the stack, at a point where the stack might still be accessed.
Taking an exception can cause the stack to be corrupted.

As a first approach, this patch is overly conservative, assuming that any instruction that may load or store could access
the stack.

Diff Detail

Repository
rL LLVM

Event Timeline

dnsampaio created this revision.Jun 11 2019, 10:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2019, 10:46 AM
lebedev.ri resigned from this revision.Jun 11 2019, 10:51 AM
thegameg edited reviewers, added: qcolombet; removed: lebedev.ri.Jun 11 2019, 11:00 AM

I agree this needs to be fixed.

I tried this almost a year ago and I saw some regressions on SPEC:

82.sphinx3/482.sphinx3         +0.46%
255.vortex/255.vortex            +2.39%
175.vpr/175.vpr                      +0.54%
124.m88ksim/124.m88ksim   +1.05%
147.vortex/147.vortex            +2.13%

But I never got the time to look into making it a bit smarter.

Maybe it's ok to ignore the regressions for now.

dnsampaio updated this revision to Diff 204240.Jun 12 2019, 2:33 AM
  • - Added specific shrink wrapping test for pr37472

Hi @thegameg,
My initial thoughts on how to optimize this was to use alias analysis to check that load and stores for sure do not alias with the stack of the current function. It seems that the current api accepts two memory operands, and then turns them into memory regions. I don't know if it possible, as we might not know the sp value it self, but my idea was to create a function that accepts a memory access and a memory region, which would be set to [SP, SP + stackSize].
In pr42136, @eli.friedman also mentioned that might be quite simple to prove that a load accesses a global variable, as well possible to prove that the address of some particular stack objects doesn't escape (also pointed by @chill) .

We also see some performance regressions in some of our tests, but we believe it is better to first fix the bug, and further think on ways to optimize it.

I'd prefer to take this soon, and deal with the performance later, given this is a serious bug. I wouldn't be surprised if this is actually causing issues in practice, but nobody realized it because it's non-deterministic.

lib/CodeGen/ShrinkWrap.cpp
261 ↗(On Diff #204240)

"popping".

262 ↗(On Diff #204240)

"aggressive".

qcolombet accepted this revision.Jun 12 2019, 12:56 PM

Agree, the fix is more important than the performance loss at this point.

LGTM, but comments to address below.

lib/CodeGen/ShrinkWrap.cpp
267 ↗(On Diff #204240)

I don't think we use C style comment usually.
Could you turn that into C++ (//) ones?

test/CodeGen/AArch64/pr37472.ll
2 ↗(On Diff #204240)

If at all possible a MIR test would be more robust here.
Otherwise, use opt -instnamer to get rid of the implicit variables.

Also:

  • please use a more explicit name for the filename. E.g., dont-shrink-wrap-for-stack-access.ll
  • put the PR number in the comment of the test
  • add a comment in the test describing the characteristic for the test: some load store that can access the stack
This revision is now accepted and ready to land.Jun 12 2019, 12:56 PM
dnsampaio marked 4 inline comments as done.Jun 13 2019, 3:45 AM

Thanks for the reviews. I'm changing the new test to take a mir obtained just before shrink-wrap pass and validating just that one pass.

This revision was automatically updated to reflect the committed changes.

Is there any fix patch proposed to track/fix the regression? @dnsampaio

Is there any fix patch proposed to track/fix the regression? @dnsampaio

Hi @wuzish. There are no proposals for this that I'm aware at the moment.

fhahn added a subscriber: fhahn.May 2 2023, 10:35 AM

I put up D149668 which tries to use getUnderlyingObject to try to prove that the stack of the current function isn't accessed.

Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2023, 10:35 AM