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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
