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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/include/llvm/Analysis/ObjCARCRVAttr.h:28
+
+static inline const char *getRVAttrValStr(bool Retain) {
+  return Retain ? "retain" : "claim";
----------------
Is there a place the attributes could be documented?


================
Comment at: llvm/lib/IR/Instruction.cpp:580
+    if (auto *CB = dyn_cast<CallBase>(this))
+      return objcarc::hasRetainRVOrClaimRVAttr(CB);
+    return false;
----------------
rjmccall wrote:
> nikic wrote:
> > This change looks pretty fishy. Objective C shouldn't be hijacking LLVMs core instruction model in this way. If it writes to memory, this should either be reflected in the attributes, or modeled using operand bundles.
> > 
> > @fhahn Did you review these changes? If not, I'd suggest to revert this patch and get a review on the LLVM changes.
> This could definitely be an operand bundle, and I suppose the presence of a bundle does force a conservative assumption on passes.
> @fhahn Did you review these changes? 

Nope I didn't have time to look at this so far.

<snip>

Can functions marked as `readonly`/`readnone` be called with the objc attributes? 

I'm not very familiar with ObjC, but even for a function that just returns a passed in object id, don't we need to retain & release the object in the function? Which means the function cannot be `readonly` because we need to call `@llvm.objc*` functions? If that's the case, could we just check in the verifier that the attributes are never used with `readonly` functions?

If there are indeed cases where we can call `readonly` functions, operand bundles would probably be safest. It would probably also good to have at least a few alias-analysis tests, to make sure things work as expected.


================
Comment at: llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp:576
+    // Don't change the return type of the function if it has retainRV/claimRV.
+    if (objcarc::hasRetainRVOrClaimRVAttr(CB)) {
+      NumLiveRetVals = RetCount;
----------------
This seems fragile. IIUC this workaround is needed because the return value of a `retainRV/claimRV` function always has one implicit use. This aspect could get missed in other places as well.

As an alternative, could the frontend insert 'dummy' uses (e.g. `llvm.objc.arc.use`) to make this assumption transparent? Those could be removed sufficiently late. Their placement doesn't really matter and neither does if any instructions are moved between the original call and the dummy use, but the fact that the returned value is used is now explicit. IICU they could be marked as accessing inaccessible memory only, so they should be too much trouble for other optimizations (at least not more than the previous explicit release calls). Or perhaps there's a different way to mark the return value as used explicitly.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92808