ahatanak added inline comments.
================
Comment at: llvm/lib/IR/Instruction.cpp:580
+ if (auto *CB = dyn_cast<CallBase>(this))
+ return objcarc::hasRetainRVOrClaimRVAttr(CB);
+ return false;
----------------
fhahn wrote:
> 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.
A function compiled using ARC can call a function compiled using MRR, which can
be readonly/readnone. Also, a function compiled using ARC can be marked as
readonly/readnone (if an optimization pass wants to do so) after ARC optimizer
removes the retainRV/autoreleaseRV pair.
```
define i8* @foo() {
%1 = tail call i8* @readonlyMRRFunc()
; This function can be readonly if ARC optimizer removes the following two
instructions.
%2 = call i8* @llvm.objc.retainAutoreleasedReturnValue(i8* %1)
%3 = tail call i8* @llvm.objc.autoreleaseReturnValue(i8* %2)
ret i8*
}
```
================
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;
----------------
fhahn wrote:
> 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.
>
I think we can make the front-end insert a dummy use instruction. The front-end
currently emits `@llvm.objc.clang.arc.use` in some cases to make sure the
object doesn't get released too early, but we can't use that in this case since
ARC optimizer treats a call to `@llvm.objc.clang.arc.use` as a real use, so we
probably need a new intrinsic.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92808/new/
https://reviews.llvm.org/D92808
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
