This is an archive of the discontinued LLVM Phabricator instance.

GlobalISel: Fix marking byval arguments as immutable
ClosedPublic

Authored by arsenm on Mar 7 2021, 11:32 AM.

Details

Summary

byval arguments need to be assumed writable. Only implicitly stack
passed arguments which aren't addressable in the IR can be assumed
immutable.

Mips is still broken since for some reason its doing its own thing
with the ValueHandlers (and x86 doesn't actually handle byval
arguments now, although some of the code is there).

Diff Detail

Event Timeline

arsenm created this revision.Mar 7 2021, 11:32 AM
arsenm requested review of this revision.Mar 7 2021, 11:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2021, 11:32 AM
Herald added a subscriber: wdng. · View Herald Transcript

Am I misunderstanding the meaning of byval? From the langref:

The attribute implies that a hidden copy of the pointee is made between the caller and the callee, so the callee is unable to modify the value in the caller. This attribute is only valid on LLVM pointer arguments. It is generally used to pass structs and arrays by value, but is also valid on pointers to scalars. The copy is considered to belong to the caller not the callee (for example, readonly functions should not write to byval parameters).

From that, I'd expect byval stack slots to be *immutable*.

arsenm added a comment.Mar 7 2021, 2:48 PM

Am I misunderstanding the meaning of byval? From the langref:

The attribute implies that a hidden copy of the pointee is made between the caller and the callee, so the callee is unable to modify the value in the caller. This attribute is only valid on LLVM pointer arguments. It is generally used to pass structs and arrays by value, but is also valid on pointers to scalars. The copy is considered to belong to the caller not the callee (for example, readonly functions should not write to byval parameters).

From that, I'd expect byval stack slots to be *immutable*.

It's unable to modify the *original* value in the caller. The caller is responsible for copying the incoming value into a scratchpad the callee is allowed to write into. I'm not aware of any situations where the IR will produce writes into a byval area, but it is technically allowed. SelectionDAG does not mark byval arguments as immutable. I also believe GlobalISel is broken since the byval lowering is currently not inserting the required copy in the call setup

aemerson accepted this revision.Mar 9 2021, 10:51 AM
This revision is now accepted and ready to land.Mar 9 2021, 10:51 AM