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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/CodeGen/CGObjC.cpp:2342
+    attrs = attrs.addAttribute(CGF.getLLVMContext(),
+                               llvm::AttributeList::ReturnIndex, "rv_marker");
+    callBase->setAttributes(attrs);
----------------
ahatanak wrote:
> 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.
> I think you are asking why both retain/claim and rv_marker are needed?

Yes.

> 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.

Is there a situation where we emit the marker but not a following call?  Or is that just in the places where we've already made the following call explicit in IR?  Because in the latter case, presumably the following call will still act as a barrier to anything that's querying `mayWriteToMemory`, right?  (That is: in general, I expect that nobody just asks whether a specific instruction can write to memory, they ask if any of the instructions in a region can write to memory, and since the following call can...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92808