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