[PATCH] D92808: [ObjC][ARC] Annotate calls with attributes instead of emitting retainRV or claimRV calls in the IR

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.


================
Comment at: clang/lib/CodeGen/CGObjC.cpp:2342
+    attrs = attrs.addAttribute(CGF.getLLVMContext(),
+                               llvm::AttributeList::ReturnIndex, "rv_marker");
+    callBase->setAttributes(attrs);
----------------
rjmccall wrote:
> It's weird that these attributes use two different capitalization styles.  Also, why are they both needed?  Did you consider making the role (retain/claim) be the value of the attribute rather than a separate attribute?
> 
> Should the attribute be namespaced, like `clang.arc.rv_marker`?
> 
> Let's go ahead and add globals for these strings so we can refer to them symbolically, like you did with `retainRVMarkerKey`.  Is there an LLVM header for ARC optimization we could reasonably pull them from, or are we doomed to repeat ourselves across projects?
I think you are asking why both `retain/claim` and `rv_marker` are needed? Or are you asking why both 'retain' and 'claim' are needed?

We could do without `clang.arc.rv_marker` and just use `clang.arc.rv=retain/claim` since the middle-end and backend passes can easily determine whether a marker is needed or not. The only drawback to using only `clang.arc.rv` I can think of is that it's no longer possible to tell whether an instruction is implicitly followed by marker+retain/claimRV or just the marker, which makes `Instruction::mayWriteToMemory` return a conservative answer if the function call is read-only. A read-only call cannot be treated as read-only if it's followed by marker+retain/claimRV, but it is still read-only if it is followed just by the marker.

Note that ARC contract pass emits the retain/claimRV instruction into the IR and drops the corresponding `clang.arc.rv` attribute but doesn't remove the `clang.arc.rv_marker` attribute. So if we used only `clang.arc.rv`, a call annotated with the attribute would be implicitly followed by marker+retain/claimRV before ARC contract, while it would be followed by just the marker after ARC contract, but `Instruction::mayWriteToMemory` wouldn't' be able to tell the difference.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92808/new/

https://reviews.llvm.org/D92808