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: > 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...) I think you are right. The retainRV/claimRV instruction that follows the annotated call will act as a barrier, so using `clang.arc.rv_marker` to distinguish between marker+retain/claimRV and marker only probably won't help the optimizations in practice. 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
