This is an archive of the discontinued LLVM Phabricator instance.

[ShrinkWrap] Use underlying object to rule out stack access.
ClosedPublic

Authored by fhahn on May 2 2023, 10:34 AM.

Details

Summary

Allow shrink-wrapping past memory accesses that only access globals or
function arguments. This patch uses getUnderlyingObject to try to
identify the accessed object by a given memory operand. If it is a
global or an argument, it does not access the stack of the current
function and should not block shrink wrapping.

Note that the caller's stack may get accessed when passing an argument
via the stack, but not the stack of the current function.

This addresses part of the TODO from D63152.

Diff Detail

Event Timeline

fhahn created this revision.May 2 2023, 10:34 AM
fhahn requested review of this revision.May 2 2023, 10:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2023, 10:34 AM
thegameg accepted this revision.May 2 2023, 11:32 AM

lgtm thanks

llvm/lib/CodeGen/ShrinkWrap.cpp
271

Unused?

This revision is now accepted and ready to land.May 2 2023, 11:32 AM

So the idea here is that things that are allocated before entry to the function can't point into the stack of the current function? The basic idea makes sense.

I'm a little concerned about the interaction with byval/preallocated/inalloca arguments, which are, depending on the calling convention, arguably part of the current function's stack. Any thoughts there?

fhahn edited the summary of this revision. (Show Details)May 2 2023, 1:30 PM
fhahn updated this revision to Diff 518846.May 2 2023, 1:34 PM

Update remaining failing tests, remove dead Objs variable.

So the idea here is that things that are allocated before entry to the function can't point into the stack of the current function? The basic idea makes sense.

I'm a little concerned about the interaction with byval/preallocated/inalloca arguments, which are, depending on the calling convention, arguably part of the current function's stack. Any thoughts there?

Thanks, I added a !Arg->hasPassPointeeByValueCopyAttr() check to catch those. Tests for with those attributes have been added in bc1c95d973b80d7ec7a15d4c5c5248952fc39153.

fhahn added inline comments.May 2 2023, 1:39 PM
llvm/lib/CodeGen/ShrinkWrap.cpp
271

Yep, removed, thanks!

This revision was landed with ongoing or failed builds.May 3 2023, 1:29 AM
This revision was automatically updated to reflect the committed changes.
jpenix-quic added inline comments.
llvm/lib/CodeGen/ShrinkWrap.cpp
286

Hi @fhahn, does this work as expected when MI.memoperands() is empty? If I understand correctly, all_of will return true in that case which I don't think is what we want (my understanding is we need to be conservative if memoperands() is empty). That said, I struggled to find a test where this causes an issue here, so I'm not sure if this is actually an issue in practice or if any possible situations are covered by the other checks, etc.

Do you have thoughts on this? If it is an issue, I imagine we could just add an extra check for the empty case or something similar.

fhahn marked an inline comment as done.May 12 2023, 8:16 AM
fhahn added inline comments.
llvm/lib/CodeGen/ShrinkWrap.cpp
286

@jpenix-quic I had a closer look and added a case where missing memoperands cause incorrect results. It's a bit artificial as memory operands are dropped from instructions that usually have them, but it illustrates the problem: ac9e9594af37

I also pushed a fix to treat instructions without memoperands conservatively: d0718ff410cc

jpenix-quic added inline comments.May 12 2023, 8:40 AM
llvm/lib/CodeGen/ShrinkWrap.cpp
286

Great, thank you for looking into and handling this!