This is an archive of the discontinued LLVM Phabricator instance.

[ObjC] Disallow vector parameters and return values in Objective-C methods on older X86 targets
ClosedPublic

Authored by arphaman on Jan 13 2017, 3:56 AM.

Details

Summary

This patch adds a new error that disallows methods that have parameters/return values with a vector type on certain older X86 targets. This is diagnostic is needed because objc_msgSend doesn't support SIMD vector registers/return values on X86 in iOS < 9 and OS X < 10.11.

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman updated this revision to Diff 84276.Jan 13 2017, 3:56 AM
arphaman retitled this revision from to [ObjC] Disallow vector parameters and return values in Objective-C methods on older X86 targets.
arphaman updated this object.
arphaman set the repository for this revision to rL LLVM.
arphaman added a subscriber: cfe-commits.

Hi Alex, thanks for CCing me!

include/clang/Basic/DiagnosticSemaKinds.td
1157 ↗(On Diff #84276)

s/return value/return type/? Also, It would be nice to mention what isn't supported (vector types) and the version that it becomes supported in.

lib/Sema/SemaDeclObjC.cpp
4337 ↗(On Diff #84276)

I wonder if this is the right place to check this. If we're compiling using a new SDK, but deploying back before vector types were supported we will diagnose a method declared in a header even if it is never called. We should probably hook this into DiagnoseAvailabilityOfDecl (in SemaExpr.cpp), which is called whenever a method is actually used, and diagnose it then.

arphaman updated this revision to Diff 84564.Jan 16 2017, 8:12 AM
arphaman edited edge metadata.
arphaman marked an inline comment as done.

Use better diagnostic message as suggested by Erik

lib/Sema/SemaDeclObjC.cpp
4337 ↗(On Diff #84276)

I'm not sure I get your argument.. We don't diagnose on method declarations, just method definitions, so I don't see how the header stuff is relevant here. We definitely don't want to diagnose on method uses either.

bruno added a subscriber: bruno.Mar 6 2017, 5:44 PM

Hi Alex,

lib/Sema/SemaDeclObjC.cpp
4312 ↗(On Diff #84564)

Assuming objc/c++ can pass/return these, the current check wouldn't be enough to cover x86_64 ABI cases where small (< 8 bytes) trivial classes containing vector types are directly passed / returned as vector types. You might want to check if that applies here.

See test/CodeGenCXX/x86_64-arguments-avx.cpp for examples (and x86_64 abi doc, item 3.2.3)

test/SemaObjC/x86-method-vector-values.m
16 ↗(On Diff #84564)

Can you also add to the testcase instances that use regular non-ext vector (vector_type(...))?

arphaman updated this revision to Diff 90832.Mar 7 2017, 4:02 AM
arphaman marked an inline comment as done.

Add a test for non-ext vector type

lib/Sema/SemaDeclObjC.cpp
4312 ↗(On Diff #84564)

It seems that on X86 all the troubling aggregates are passed by pointer anyway, and we don't warn for X86_64 at all. The only aggregates that seem to use registers for Objective-C methods are the ones that don't use vector registers.

bruno added inline comments.Apr 12 2017, 4:42 PM
lib/Sema/SemaDeclObjC.cpp
4309 ↗(On Diff #90832)

Maybe add an assert for Triple.getArch() == llvm::Triple::x86) here?

arphaman updated this revision to Diff 95099.Apr 13 2017, 4:09 AM
arphaman marked an inline comment as done.

Add an assertion as requested by Bruno.

bruno accepted this revision.Apr 26 2017, 6:56 PM

Thanks Alex. LGTM

This revision is now accepted and ready to land.Apr 26 2017, 6:56 PM
This revision was automatically updated to reflect the committed changes.