Page MenuHomePhabricator

[CodeGen][ObjC] Add attribute "objc_arc_intert" to ObjC globals that are retain-agnostic
ClosedPublic

Authored by ahatanak on Jun 3 2019, 4:18 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

ahatanak created this revision.Jun 3 2019, 4:18 PM
lib/CodeGen/CGBlocks.cpp
1441 ↗(On Diff #202820)

Can't you just declare literal as a GlobalVariable instead of immediately casting it?

lib/CodeGen/CodeGenModule.cpp
4654 ↗(On Diff #202820)

cast seems unnecessary.

test/CodeGenObjC/local-static-block.m
5 ↗(On Diff #202820)

I believe FileCheck ignores CHECK lines when there is a custom check-prefix, so this should be CHECK-LP64. I think this makes more sense regardless in its own file though, its weird to piggyback on an unrelated test IMO.

59–60 ↗(On Diff #202820)

(Likewise, these should be CHECK-LP64, or this test shouldn't be using a check-prefix. Would you mind updating this?)

ahatanak marked 2 inline comments as done.Jun 5 2019, 2:09 PM
ahatanak added inline comments.
test/CodeGenObjC/local-static-block.m
59–60 ↗(On Diff #202820)

Fixed in r362651.

ahatanak updated this revision to Diff 203247.Jun 5 2019, 2:15 PM
ahatanak marked 5 inline comments as done.

Address review comments.

ahatanak added inline comments.Jun 5 2019, 2:18 PM
test/CodeGenObjC/local-static-block.m
5 ↗(On Diff #202820)

I added a file that just tests this attribute.

I think this looks good from a clang perspective. No reason to land it before the dust settles on D62433 though.

Do you think we can also add this attribute (or something like it) onto allocas of _NSConcreteStackBlocks? They also have this retain-swallowing behaviour.

This is exactly what I was imagining! This will enable the frontend to opt into this optimization without having to touch the optimizer. One nit: can we use a different name than "arc_retain_agnostic". Have you considered something like "arc_inert"? My fear is that at a glance (without thinking), you would think that the attribute would have something only to do with retain when we are really talking about ARC value operations. Beyond that looks great!

I agree. Something like arc_inert is probably a better name in this case for the reason you mentioned.

Should objc be in the attribute somewhere to avoid any future awkwardness? I don't remember what we've called similar attributes.

ahatanak updated this revision to Diff 204705.Jun 13 2019, 11:00 PM
ahatanak retitled this revision from [CodeGen][ObjC] Add attribute "arc_retain_agnostic" to ObjC globals that are retain-agnostic to [CodeGen][ObjC] Add attribute "objc_arc_intert" to ObjC globals that are retain-agnostic.

There are many attributes that start with objc_ including objc_arc_weak_reference_unavailable.

Please rename the test. There's also a typo in the differential title; please make sure that doesn't get into the commit message. Otherwise LGTM.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 14 2019, 3:04 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2019, 3:04 PM