[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

fhahn accepted this revision. fhahn added a comment. This revision is now accepted and ready to land. LGTM as initial step, but it would be good to adjust the name as Duncan suggested before landing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

ahatanak added a comment. In D92808#2557132 , @fhahn wrote: > Another thing I noticed that there's verifier support missing. I think we > should at least check that only a single `clang.arc.rv` bundle is specified >

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

ahatanak updated this revision to Diff 323107. ahatanak added a comment. Address all review comments except for the ones about the bundle name. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92808/new/ https://reviews.llvm.org/D92808 Files:

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

ahatanak marked 3 inline comments as done. ahatanak added a comment. In D92808#2557634 , @dexonsmith wrote: > In D92808#2555868 , @ahatanak wrote: > >> For example, if SCCP just does a normal RAUW on the following

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

dexonsmith added a comment. In D92808#2556542 , @fhahn wrote: > @dexonsmith @rjmccall What do you think? Would you be happy with iterating > on the suggestions in tree? I don't feel strongly either way; happy for it to land as-is (bugs fixed) and

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

dexonsmith added a comment. In D92808#2555868 , @ahatanak wrote: > For example, if SCCP just does a normal RAUW on the following call, which is > taken from the example I posted, > > %r = call i8* @foo() [ "clang.arc.rv"(i64 1, i8* %r) ] > > will

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

fhahn added a comment. Another thing I noticed that there's verifier support missing. I think we should at least check that only a single `clang.arc.rv` bundle is specified (https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/Verifier.cpp#L3191). We should probably also enforce that the

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

fhahn added a comment. In D92808#2555868 , @ahatanak wrote: > In D92808#2555737 , @dexonsmith > wrote: > >> In D92808#2552354 , @rjmccall wrote: >> >>> The ultimate

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

ahatanak reopened this revision. ahatanak added a comment. This revision is now accepted and ready to land. In D92808#2555737 , @dexonsmith wrote: > In D92808#2552354 , @rjmccall wrote: > >> The ultimate

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

ahatanak updated this revision to Diff 322867. ahatanak added a comment. Fix two bugs which were discovered. - Fix bug in `replaceUsesOfNonProtoConstant`. - Teach SCCP not to replace the return of a "clang.arc.rv" call with a constant. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

dexonsmith added a comment. In D92808#2552354 , @rjmccall wrote: > The ultimate code-generation consequences of this feature aren't very > generic, so we shouldn't let ourselves get caught up designing an extremely > general feature that ultimately

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

rjmccall added a comment. The ultimate code-generation consequences of this feature aren't very generic, so we shouldn't let ourselves get caught up designing an extremely general feature that ultimately either doesn't do what we need it to do or doesn't actually work for any of the

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

dexonsmith added a comment. I have a couple of suggestions below to add to the mix (but I don't feel strongly about either of them, just adding them for consideration). In D92808#2552100 , @ahatanak wrote: > define void @test() { > %r = call i8*

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

ahatanak added a comment. This patch causes opt to crash when the following IR is compiled: $ cat test.ll @g0 = global i8 zeroinitializer, align 1 define internal i8* @foo() { ret i8* @g0 } define void @test() { %r = call i8* @foo() [ "clang.arc.rv"(i64 1) ] call void

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

ahatanak added a comment. It looks like there is a pre-existing bug in `replaceUsesOfNonProtoConstant` where the operand bundles of all call sites are accumulated into `newBundles`. This will be fixed if its declaration is moved into the loop body. Also, we found another case of deadargelim

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

thakis added a comment. Here's a reduced repro: thakis@thakis:~/src/llvm-project$ cat GREYAssertDefaultConfiguration-9e1f3b.reduced.m inline id a() {} void b() { a(); a(); } thakis@thakis:~/src/llvm-project$ out/gn/bin/clang -cc1 -cc1 -triple arm64-apple-ios12.2.0 -Oz

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

thakis added a comment. This makes clang assert for some inputs. See https://bugs.chromium.org/p/chromium/issues/detail?id=1175886#c10 for a repro. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92808/new/ https://reviews.llvm.org/D92808

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

ahatanak added a comment. I do have a plan to modify the auto upgrader to use the bundles when it's possible to do so, but I'm currently not considering landing the changes I reverted since they were only needed to avoid duplicating constant strings. I don't think we can avoid duplicating the

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

dexonsmith added a comment. In D92808#2545873 , @ahatanak wrote: > I ended up reverting the changes I made to `llvm/lib/IR/AutoUpgrade.cpp` as > the file was including `llvm/Analysis/ObjCARCUtil.h`, which was violating > layering. Are you planning to

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

ahatanak added a comment. I ended up reverting the changes I made to `llvm/lib/IR/AutoUpgrade.cpp` as the file was including `llvm/Analysis/ObjCARCUtil.h`, which was violating layering. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92808/new/

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG3fe3946d9a95: [ObjC][ARC] Use operand bundle clang.arc.rv instead of explicitly (authored by ahatanak). Repository: rG LLVM Github Monorepo

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

fhahn accepted this revision. fhahn added a comment. This revision is now accepted and ready to land. Core LLVM parts LGTM, thanks! I'm going to mark this as accepted, given that the other parts have been reviewed earlier already Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

ahatanak updated this revision to Diff 320985. ahatanak added a comment. Add a description of the new bundle to LangRef. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92808/new/ https://reviews.llvm.org/D92808 Files:

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

ahatanak updated this revision to Diff 320956. ahatanak marked 3 inline comments as done. ahatanak added a comment. Address review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92808/new/ https://reviews.llvm.org/D92808 Files:

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

fhahn added a comment. Thanks for the updates! The LLVM side of things now looks good to me. Could you also add a description of the new bundle to LangRef https://llvm.org/docs/LangRef.html#operand-bundles? In D92808#2535593 , @ahatanak wrote: > We

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

ahatanak added a comment. We could run another pass to clean up the IR after inlining, but I think it's better to do the cleanup right after the callee function is cloned in the caller function. Comment at: llvm/include/llvm/IR/Intrinsics.td:449

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

ahatanak updated this revision to Diff 320663. ahatanak marked 4 inline comments as done. ahatanak added a comment. Address review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92808/new/ https://reviews.llvm.org/D92808 Files:

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

fhahn added a comment. Thanks for the update! The approach with operand bundles and the no-op uses looks cleaner than the original version. The special handling in the inliner seems a bit unfortunate, but I guess there's no way around that? (as a side note, it might be easier to submit if this

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

ahatanak updated this revision to Diff 320001. ahatanak added a comment. Herald added a subscriber: laytonio. Teach `markTails` to ignore operand bundle `clang.arc.rv` when determining whether a call can be marked as tail. `ObjCARCOpt::OptimizeReturn` cannot eliminate the retainRV/autoreleaseRV

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

ahatanak reopened this revision. ahatanak added a comment. This revision is now accepted and ready to land. Address post-commit review comments. - Use operand bundle instead of attribute. - Emit a call to `@llvm.objc.clang.arc.noop.use` in the front-end so that the optimization passes know the

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

ahatanak updated this revision to Diff 319719. ahatanak retitled this revision from "[ObjC][ARC] Annotate calls with attributes instead of emitting retainRV or claimRV calls in the IR" to "[ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in