This is an archive of the discontinued LLVM Phabricator instance.

IR: Reject unsized sret in verifier
Needs ReviewPublic

Authored by arsenm on Sep 25 2020, 11:28 AM.

Details

Summary

The same is enforced for the other ABI pointer attributes.

Not sure why MemCpyOpt/memcpy.ll seems to have been testing for
exactly this with a referenced radar number, although I don't see how
that makes sense.

Diff Detail

Event Timeline

arsenm created this revision.Sep 25 2020, 11:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2020, 11:28 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
arsenm requested review of this revision.Sep 25 2020, 11:28 AM

I'm in favor.

llvm/test/Transforms/MemCpyOpt/memcpy.ll
365

FWIW, this was introduced in bd254f260127 and I cannot tell if such code should even be legal in the first place (and this patch says its not)

sret marks that a parameter needs the special indirect-return argument ABI, which does *not* require a sized type on many architectures. Swift actually relies on being able to trigger this ABI on a value of unknown size. Swift can, of course, just use a meaningless sized type, but since I assume you intend to actually use the type for something, I am concerned.

sret marks that a parameter needs the special indirect-return argument ABI, which does *not* require a sized type on many architectures. Swift actually relies on being able to trigger this ABI on a value of unknown size. Swift can, of course, just use a meaningless sized type, but since I assume you intend to actually use the type for something, I am concerned.

Well, I'm concerned this edge case apparently exists and has very little test coverage anywhere. It's the odd one out among the other type carrying attributes and I dislike the asymmetry with byval. Is there any real reason to use an unsized type over a 0 sized type here?

Herald added a project: Restricted Project. · View Herald TranscriptNov 16 2022, 7:41 AM

Inconsistency between things that are different is expected. byval intrinsically requires a sized type because the compiler has to know how large of an object to allocate + copy in the argument area. sret does not require a type on any target I know of, and if there are places that expect the type to be meaningful, they are almost certainly buggy in the presence of dynamically-sized types. While C has only very restricted support for dynamically-sized types, I believe Fortran's support is broader, and so psABIs usually nod at them in various ways — it's not something that's novel to Swift.

What do we use the sret type for, anyway? I tried looking around, but the only actual use I can find is Value::getPointerAlignment. (SelectionDAG stores the type, but doesn't actually use it for anything, as far as I can tell. And I'm not seeing any other usage.) We should probably consider just dropping the type from sret.

What do we use the sret type for, anyway? I tried looking around, but the only actual use I can find is Value::getPointerAlignment. (SelectionDAG stores the type, but doesn't actually use it for anything, as far as I can tell. And I'm not seeing any other usage.) We should probably consider just dropping the type from sret.

objectsize looks at the allocated size at least.

If the type is just used for optimization purposes, we have other attributes that work more generally for pointer-typed arguments.

llvm/lib/IR/Verifier.cpp