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

[ahatanak]

Summary

Background:

This fixes a longstanding problem where llvm breaks ARC's autorelease optimization (see the link below) by separating calls from the marker instructions or retainRV/claimRV calls. The backend changes are in https://reviews.llvm.org/D92569.

https://clang.llvm.org/docs/AutomaticReferenceCounting.html#arc-runtime-objc-autoreleasereturnvalue

What this patch does to fix the problem:

• The front-end adds operand bundle "clang.arc.attachedcall" to calls which indicates the call is implicitly followed by a marker instruction and an implicit retainRV/claimRV call that consumes the call result. In addition, it emits a call to @llvm.objc.clang.arc.noop.use, which consumes the call result, to prevent the middle-end passes from changing the return type of the called function. This is currently done only when the target is arm64 and the optimization level is higher than -O0.

• ARC optimizer temporarily emits retainRV/claimRV calls after the calls with the operand bundle in the IR and removes the inserted calls after processing the function.

• ARC contract pass emits retainRV/claimRV calls after the call with the operand bundle. It doesn't remove the operand bundle on the call since the backend needs it to emit the marker instruction. The retainRV/claimRV calls are emitted late in the pipeline to prevent optimization passes from transforming the IR in a way that makes it harder for the ARC middle-end passes to figure out the def-use relationship between the call and the retainRV/claimRV calls (which is the cause of PR31925).

• The function inliner removes an autoreleaseRV call in the callee if nothing in the callee prevents it from being paired up with the retainRV/claimRV call in the caller. It then inserts a release call if claimRV is attached to the call since autoreleaseRV+claimRV is equivalent to a release. If it cannot find an autoreleaseRV call, it tries to transfer the operand bundle to a function call in the callee. This is important since the ARC optimizer (ObjCARCOpt::OptimizeReturns) can remove the autoreleaseRV returning the callee result, which makes it impossible to pair it up with the retainRV/claimRV call in the caller. If that fails, it simply emits a retain call in the IR if retainRV is attached to the call and does nothing if claimRV is attached to it.

• SCCP refrains from replacing the return value of a call with a constant value if the call has the operand bundle. This ensures the call always has at least one user (the call to @llvm.objc.clang.arc.noop.use), which is necessary to prevent passes like deadargelim from changing the return type.

• This patch also fixes a bug in replaceUsesOfNonProtoConstant where multiple operand bundles of the same kind were being added to a call.

Future work:

• Use the operand bundle on x86-64.

• Fix the auto upgrader to convert call+retainRV/claimRV pairs into calls with the operand bundles.

rdar://71443534

[fhahn]

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 would be split up in the changes adding the bundle, followed by the ARC optimization & clang changes)

llvm/include/llvm/Analysis/ObjCARCUtil.h

41

I think we should include LLVMContext.h explicitly?

llvm/include/llvm/IR/InstrTypes.h

1217

Please add documentation for the new functions.

llvm/include/llvm/IR/Intrinsics.td

449

Can this be a DefaultAttrIntrinsics (which adds nofree nosync nounwind willreturn)?

Same probably for all/most objc_* intrinsics.

llvm/lib/Transforms/Utils/InlineFunction.cpp

1650

Could you add a comment elaborating what this function does and why it is needed?

[ahatanak]

Address review comments.

[ahatanak]

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.

llvm/include/llvm/IR/Intrinsics.td

449

I'll change the other intrinsics in a separate patch.

[fhahn]

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 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.

Thanks for elaborating. Given that it is isolated to a single place and the inliner already has logic to deal with a few different intrinsics/bundle types, I think that should be fine.

llvm/include/llvm/IR/Intrinsics.td

449

SGTM. thanks!

llvm/lib/Transforms/Utils/InlineFunction.cpp

1655

nit: typo autoreleasaeRV

1669

nit: Can you just use CB.getModule()?

1673

we are in the llvm:: namespace here I think, so this should not be needed ? above you use the objcarc namespace without llvm:: prefix as well. Same in other places in the function.

[ahatanak]

Address review comments.

[ahatanak]

Add a description of the new bundle to LangRef.

[fhahn]

Core LLVM parts LGTM, thanks! I'm going to mark this as accepted, given that the other parts have been reviewed earlier already

[ahatanak]

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.

[dexonsmith]

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 land the upgrade another way?

[ahatanak]

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 string unless objcarc::getRVMarkerModuleFlagStr() can be moved to a file in llvm/include/llvm/IR.

[• thakis]

This makes clang assert for some inputs. See https://bugs.chromium.org/p/chromium/issues/detail?id=1175886#c10 for a repro.

[• thakis]

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 -fobjc-arc -emit-llvm GREYAssertDefaultConfiguration-9e1f3b.reduced.m
GREYAssertDefaultConfiguration-9e1f3b.reduced.m:1:16: warning: non-void function does not return a value [-Wreturn-type]
inline id a() {}
               ^
clang: ../../llvm/include/llvm/IR/InstrTypes.h:1969: Optional<llvm::OperandBundleUse> llvm::CallBase::getOperandBundle(uint32_t) const: Assertion `countOperandBundlesOfType(ID) < 2 && "Precondition violated!"' failed.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.	Program arguments: out/gn/bin/clang -cc1 -cc1 -triple arm64-apple-ios12.2.0 -Oz -fobjc-arc -emit-llvm GREYAssertDefaultConfiguration-9e1f3b.reduced.m
1.	<eof> parser at end of file
2.	Per-module optimization passes
3.	Running pass 'CallGraph Pass Manager' on module 'GREYAssertDefaultConfiguration-9e1f3b.reduced.m'.
 #0 0x00000000034a054c llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /usr/local/google/home/thakis/src/llvm-project/out/gn/../../llvm/lib/Support/Unix/Signals.inc:565:13
 #1 0x000000000349e45e llvm::sys::RunSignalHandlers() /usr/local/google/home/thakis/src/llvm-project/out/gn/../../llvm/lib/Support/Signals.cpp:72:18
 #2 0x00000000034a089f SignalHandler(int) /usr/local/google/home/thakis/src/llvm-project/out/gn/../../llvm/lib/Support/Unix/Signals.inc:407:1
 #3 0x00007f8505e34140 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x14140)
 #4 0x00007f850594cce1 raise ./signal/../sysdeps/unix/sysv/linux/raise.c:51:1
 #5 0x00007f8505936537 abort ./stdlib/abort.c:81:7
 #6 0x00007f850593640f get_sysdep_segment_value ./intl/loadmsgcat.c:509:8
 #7 0x00007f850593640f _nl_load_domain ./intl/loadmsgcat.c:970:34
 #8 0x00007f8505945662 (/lib/x86_64-linux-gnu/libc.so.6+0x34662)
 #9 0x0000000002be88bd (out/gn/bin/clang+0x2be88bd)
#10 0x0000000003afdd35 hasValue /usr/local/google/home/thakis/src/llvm-project/out/gn/../../llvm/include/llvm/ADT/Optional.h:194:53
#11 0x0000000003afdd35 hasValue /usr/local/google/home/thakis/src/llvm-project/out/gn/../../llvm/include/llvm/ADT/Optional.h:286:52
#12 0x0000000003afdd35 hasRVOpBundle /usr/local/google/home/thakis/src/llvm-project/out/gn/../../llvm/include/llvm/Analysis/ObjCARCUtil.h:42:61
#13 0x0000000003afdd35 llvm::InlineFunction(llvm::CallBase&, llvm::InlineFunctionInfo&, llvm::AAResults*, bool, llvm::Function*) /usr/local/google/home/thakis/src/llvm-project/out/gn/../../llvm/lib/Transforms/Utils/InlineFunction.cpp:1953:9

Given that the repro is so tiny, I've reverted this for now in de1966e5427985163f8e816834b3a0564b5e24cd.

[ahatanak]

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 changing the return type of the called function to void. I'll post a new patch when I have the fixes.

[ahatanak]

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 (...) @llvm.objc.clang.arc.noop.use(i8* %r)
  ret void
}

declare void @llvm.objc.clang.arc.noop.use(...)

!llvm.module.flags = !{!0}

!0 = !{i32 1, !"clang.arc.retainAutoreleasedReturnValueMarker", !"mov\09fp, fp\09\09// marker for objc_retainAutoreleaseReturnValue"}

$ opt -ipsccp -deadargelim -objc-arc -o - -S test.ll

What's happening is that ipsccp replaces argument %r passed to call @llvm.objc.clang.arc.noop.use with @g0 and then deadargelim changes the return type of @foo to void since there are no explicit users of its return. ARC optimizer crashes because it expects a call with the operand bundle to have a return.

To fix the crash, we can modify tryToReplaceWithConstant to prevent ipsccp from replacing @llvm.objc.clang.arc.noop.use's argument. Alternatively, we can restore the check in DeadArgumentEliminationPass::SurveyFunction, which was in earlier versions of the patch (see https://reviews.llvm.org/D92808?id=319082#inline-892965). I feel like the latter option is better since we wouldn't' have to prevent ipsccp from optimizing the IR, which is perfectly legal. If it's not desirable to add a check that is too specific to ObjC, we could add a new generic function to the core library, which indicates the function's return type shouldn't be changed, and use it to tell deadargelim not to change the return type. Or we could use a bundle that is more generic than clang.arc.rv (for example, "implicitly.used.by"(@llvm.objc.retainAutoreleasedReturnValue), which indicates the result is used by an instruction that isn't explicitly emitted in the IR).

[dexonsmith]

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* @foo() [ "clang.arc.rv"(i64 1) ]
  call void (...) @llvm.objc.clang.arc.noop.use(i8* %r)
  ret void
}

Firstly, it feels to me like this IR would clarify that the bundle looks at the return value:

define void @test() {
  %r = call i8* @foo() [ "clang.arc.rv"(i64 1, i8* %r) ]
  ret void
}

Notice this passes %r to the operand bundle, which the verifier rejects. I wonder if the IR rule could safely be weakened to allow an operand bundle to reference the call/invoke it's attached to? (Not sure what all the implications would be, or if this would be sufficient...)

In D92808#2552100, @ahatanak wrote:

Or we could use a bundle that is more generic than clang.arc.rv (for example, "implicitly.used.by"(@llvm.objc.retainAutoreleasedReturnValue), which indicates the result is used by an instruction that isn't explicitly emitted in the IR).

Secondly, I agree it might be cleaner to formalize a generic operand bundle that has the semantics ARC needs. It might be nice to have this also model the inlining semantics, maybe something like "returnfilter":

%x = call type @f1(...) [ "returnfilter"(type(type)* @f2) ]

The semantics could be that %x is implicitly passed through @f2. Concretely, maybe that's:

• %x has an implicit use.
• @f1's return type cannot be changed.
• Inlining @f1 generates explicit calls to @f2.
• The backend lowers the bundle as late as possible to calls to @f2.

Thirdly, you could combine the two previous ideas, generalizing the semantics to an "onreturn" bundle that is allowed to self-reference the callable, that the inliner knows about, etc., something like:

%x = call type @f1(...) [ "onreturn"(type(type)* @f2, type %x) ]

This @f2 is implicitly called and its return used in place of %x, but it explicitly lists its arguments. Compared to "returnfilter", it has the flexibility to specify argument order and might be more useful for other applications.

[rjmccall]

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 generalizations.

[dexonsmith]

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 either doesn't do what we need it to do or doesn't actually work for any of the generalizations.

Sure, those are two bad potential outcomes.

Stepping back, the goal of this patch is to encode ARC's code generation invariants in the IR so that maintainers don't inadvertently break them. I think the IR should describe those invariants as clearly and precisely as is reasonable.

Besides idle / misplaced optimism about reuse, I was trying to make the following points along those lines, so I'll restate them more directly:

• Explicitly adding a use of the call avoids having to teach transformations / analyses about it. It might be simpler to add a self-reference carve-out for this operand bundle (or operand bundles in general) than to teach all the passes that this operand bundle implies a use of the callsite (but I'm not sure; maybe there are lots of assumptions about self-references not existing outside of PHIs...).
• Passing the function that's implicitly called into the operand bundle is simpler to reason about than an opaque table index.
• Naming the operand bundle after its code generation consequences makes it simpler reason about. (A few generic names that make some sense to me (besides what I listed yesterday) include "callafter", "callimmediately", "forwardto", and "attachedcall". If it's important to make this ARC only, it could be (e.g.) "clang.arc.attachedcall".)

[ahatanak]

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.

[ahatanak]

In D92808#2555737, @dexonsmith wrote:

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 either doesn't do what we need it to do or doesn't actually work for any of the generalizations.

Sure, those are two bad potential outcomes.

Stepping back, the goal of this patch is to encode ARC's code generation invariants in the IR so that maintainers don't inadvertently break them. I think the IR should describe those invariants as clearly and precisely as is reasonable.

Besides idle / misplaced optimism about reuse, I was trying to make the following points along those lines, so I'll restate them more directly:

• Explicitly adding a use of the call avoids having to teach transformations / analyses about it. It might be simpler to add a self-reference carve-out for this operand bundle (or operand bundles in general) than to teach all the passes that this operand bundle implies a use of the callsite (but I'm not sure; maybe there are lots of assumptions about self-references not existing outside of PHIs...).
• Passing the function that's implicitly called into the operand bundle is simpler to reason about than an opaque table index.
• Naming the operand bundle after its code generation consequences makes it simpler reason about. (A few generic names that make some sense to me (besides what I listed yesterday) include "callafter", "callimmediately", "forwardto", and "attachedcall". If it's important to make this ARC only, it could be (e.g.) "clang.arc.attachedcall".)

The second and third suggestions seem like an improvement over the bundle used in this patch.

I'm not sure about the first one though as it seems to me that you'll still have to teach at least some of the passes about the self-reference.

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 become

%r = call i8* @foo() [ "clang.arc.rv"(i64 1, i8* @g0) ]

%r doesn't have an explicit use in the IR anymore, so passes like deadargelim can change the return type of the function to void.

[fhahn]

In D92808#2555868, @ahatanak wrote:

In D92808#2555737, @dexonsmith wrote:

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 either doesn't do what we need it to do or doesn't actually work for any of the generalizations.

Sure, those are two bad potential outcomes.

Stepping back, the goal of this patch is to encode ARC's code generation invariants in the IR so that maintainers don't inadvertently break them. I think the IR should describe those invariants as clearly and precisely as is reasonable.

Besides idle / misplaced optimism about reuse, I was trying to make the following points along those lines, so I'll restate them more directly:

• Explicitly adding a use of the call avoids having to teach transformations / analyses about it. It might be simpler to add a self-reference carve-out for this operand bundle (or operand bundles in general) than to teach all the passes that this operand bundle implies a use of the callsite (but I'm not sure; maybe there are lots of assumptions about self-references not existing outside of PHIs...).
• Passing the function that's implicitly called into the operand bundle is simpler to reason about than an opaque table index.
• Naming the operand bundle after its code generation consequences makes it simpler reason about. (A few generic names that make some sense to me (besides what I listed yesterday) include "callafter", "callimmediately", "forwardto", and "attachedcall". If it's important to make this ARC only, it could be (e.g.) "clang.arc.attachedcall".)

The second and third suggestions seem like an improvement over the bundle used in this patch.

+1, especially passing the function to call as argument to the bundle would make the lowering slightly easier; at the moment we may need to inject a new declaration during codegen.

I'm not sure about the first one though as it seems to me that you'll still have to teach at least some of the passes about the self-reference.

The patch already tries to do that, by adding calls to llvm.objc.clang.arc.noop.use, to make it explicit that there is a use of the returned value. This ensures most optimizations are aware of the fact that the return value is used. The problem Akira is that regular uses can be updated and are not enough to block the problematic transformation in SCCP.

If the use is directly at the bundle, I think we could somehow teach to lowering to use the referenced value and pass that to the ObjC runtime. But it would disable the runtime optimization.

I think in the short-term it would be OK to update SCCP as done in this patch, if we specify the arc bundle implicitly uses the return value, which does not refer to any ObjC specifics. We already do something similar for function with certain attributes.

I think it would be good to consider generalizing this 'implicitly used return value bundle' concept as a follow up, if there are other potential users. We then already laid the ground-work and updated the existing passes to work for the arc bundle and only have to update the checks.

@dexonsmith @rjmccall What do you think? Would you be happy with iterating on the suggestions in tree?

llvm/docs/LangRef.rst

2335 ↗

(On Diff #322867)

Can you add a sentence making it explicit that calls with that bundle implicitly use the returned value?

llvm/lib/Transforms/Scalar/SCCP.cpp

148 ↗

(On Diff #322867)

I think it would be slightly simpler to have a positive name for the set, e.g. MustPreserveReturnsInFn. What do you think? IMO that's more general than referring to 'not zapping'.

1643 ↗

(On Diff #322867)

I'd keep things generic and say something like Calls with "clang.arc.rv" implicitly use the return value and those uses cannot be updated with a constant..

[fhahn]

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 bundle is only provided for functions with an i8* return type. That can also be done after the main patch lands.

[dexonsmith]

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 become

%r = call i8* @foo() [ "clang.arc.rv"(i64 1, i8* @g0) ]

%r doesn't have an explicit use in the IR anymore, so passes like deadargelim can change the return type of the function to void.

Is that a problem? It seems like this keeps all the information ARC needs. This could be lowered to:

call void @foo()
call @unsafeClaimAutoreleasedReturnValue(i8* @g0)

The ARC optimizer and ARC lowering both need to know about this case, but I don't see why deadargelim needs to know anything special.

[dexonsmith]

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 iterated on there. One thought is that if the name is going to change it'd be nice to have it right for git-blame (today I would vote for "clang.arc.attachedcall" but anything a bit more clear would be fine), but of course that can change later too.

[ahatanak]

In D92808#2557634, @dexonsmith wrote:

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 become

%r = call i8* @foo() [ "clang.arc.rv"(i64 1, i8* @g0) ]

%r doesn't have an explicit use in the IR anymore, so passes like deadargelim can change the return type of the function to void.

Is that a problem? It seems like this keeps all the information ARC needs. This could be lowered to:

call void @foo()
call @unsafeClaimAutoreleasedReturnValue(i8* @g0)

The ARC optimizer and ARC lowering both need to know about this case, but I don't see why deadargelim needs to know anything special.

I see, you are right.

[ahatanak]

Address all review comments except for the ones about the bundle name.

[ahatanak]

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 (https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/Verifier.cpp#L3191). We should probably also enforce that the bundle is only provided for functions with an i8* return type. That can also be done after the main patch lands.

I added the checks to the verifier. Note that the verifier accepts calls returning any pointer type since the return type isn't always i8* (e.g., NSObject *foo(void)).

[fhahn]

LGTM as initial step, but it would be good to adjust the name as Duncan suggested before landing.

[ahatanak]

• Renamed the operand bundle name to clang.arc.attachedcall. I also tried to pass the address of the ARC function as the argument, but the verifier doesn't allow taking addresses of intrinsics.

• Removed the call to @llvm.objc.clang.arc.noop.use, which in some cases was preventing the inliner to attach the operand bundle to a call in the callee function, when the operand bundle is removed from a call (see BundledRetainClaimRVs::eraseInst).

• Make sure emitOptimizedARCReturnCall doesn't drop any existing operand bundle on a call when adding the new operand bundle to the call.

[jyknight]

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.

It looks like it was not actually reverted in the version ultimately submitted. I've pushed commit 3c06676de14d7dd6dc3d1bf2989c503a2a5d438a which reverts the change to llvm/lib/IR/AutoUpgrade.cpp (duplicating the string constant).

[ahatanak]

Thanks! I didn't realize it hadn't been fixed in the patch I committed.

[• thakis]

FYI, we're seeing test failures caused by this patch in iOS/arm64 builds when arc is used (simulator is fine though): https://bugs.chromium.org/p/chromium/issues/detail?id=1182642#c11

We'll try to investigate a bit more on our side, but I wanted to give an early(ish) heads-up in case others see this or whatnot.

Not sure if this landed before or after the 12.0 branch.

[ahatanak]

In D92808#2593204, @thakis wrote:

FYI, we're seeing test failures caused by this patch in iOS/arm64 builds when arc is used (simulator is fine though): https://bugs.chromium.org/p/chromium/issues/detail?id=1182642#c11

We'll try to investigate a bit more on our side, but I wanted to give an early(ish) heads-up in case others see this or whatnot.

Not sure if this landed before or after the 12.0 branch.

Thank you. Let me know when you have more information.

This isn't in release/12.x. The older version of the patch, which used attributes instead of operand bundles, was in release/12.x, but got reverted later.

[smeenai]

In D92808#2593331, @ahatanak wrote:

In D92808#2593204, @thakis wrote:

FYI, we're seeing test failures caused by this patch in iOS/arm64 builds when arc is used (simulator is fine though): https://bugs.chromium.org/p/chromium/issues/detail?id=1182642#c11

We'll try to investigate a bit more on our side, but I wanted to give an early(ish) heads-up in case others see this or whatnot.

Not sure if this landed before or after the 12.0 branch.

Thank you. Let me know when you have more information.

This isn't in release/12.x. The older version of the patch, which used attributes instead of operand bundles, was in release/12.x, but got reverted later.

Just to confirm, the revert is also in release/12.x, so I don't think there's any branch concerns here.

[• thakis]

Thank you. Let me know when you have more information.

Repro is moving along. https://bugs.chromium.org/p/chromium/issues/detail?id=1182642#c26 and onward are getting pretty close.

[fhahn]

In D92808#2600031, @thakis wrote:

Thank you. Let me know when you have more information.

Repro is moving along. https://bugs.chromium.org/p/chromium/issues/detail?id=1182642#c26 and onward are getting pretty close.

That's interesting, thanks for sharing! I had a look at the linked good.s/bad.s and the sequence mentioned in the comments looks unexpected. mov x29, x29 should be exactly between a regular call and _objc_retainAutoreleasedReturnValue, but this is not what's happening in this case. I could not find a link to download the (pre-processed) source file. Would it be possible to get that file?

Ltmp239:
	bl	_objc_msgSend
Ltmp705:
	mov	x29, x29
Ltmp240:
	b	LBB19_3
LBB19_3:                                ; %invoke.cont4
	.loc	149 0 9                         ; ../../ios/chrome/browser/ui/infobars/infobar_container_coordinator.mm:0:9
	bl	_objc_retainAutoreleasedReturnValue

[hans]

The preprocessed source is attached to this comment: https://bugs.chromium.org/p/chromium/issues/detail?id=1182642#c26

good.s and bad.s are here: https://bugs.chromium.org/p/chromium/issues/detail?id=1182642#c28

And this shows the problematic difference between them: https://bugs.chromium.org/p/chromium/issues/detail?id=1182642#c30

After this change, for some reason x0 is getting clobbered in the prologue.

In https://bugs.chromium.org/p/chromium/issues/detail?id=1182642#c31 I verify that manually changing the register back to x1 fixes the issue.

[hans]

Reverted in https://github.com/llvm/llvm-project/commit/0a5dd067181dac2a8882a139ea3bd19bdea5fa44 until this can be fixed.

[fhahn]

In D92808#2600245, @hans wrote:

Reverted in https://github.com/llvm/llvm-project/commit/0a5dd067181dac2a8882a139ea3bd19bdea5fa44 until this can be fixed.

Thanks! I pushed a fix for the issue in 75805dce5ff8. @hans, would it be possible to check if D92808 + 75805dce5ff8 fixes the issue or should we just recommit D92808?

[hans]

In D92808#2601408, @fhahn wrote:

In D92808#2600245, @hans wrote:

Reverted in https://github.com/llvm/llvm-project/commit/0a5dd067181dac2a8882a139ea3bd19bdea5fa44 until this can be fixed.

Thanks! I pushed a fix for the issue in 75805dce5ff8. @hans, would it be possible to check if D92808 + 75805dce5ff8 fixes the issue or should we just recommit D92808?

Yes, I confirmed that 75805dce5ff8 fixes the test case I was looking at. Thanks!

[fhahn]

In D92808#2602687, @hans wrote:

In D92808#2601408, @fhahn wrote:

In D92808#2600245, @hans wrote:

Reverted in https://github.com/llvm/llvm-project/commit/0a5dd067181dac2a8882a139ea3bd19bdea5fa44 until this can be fixed.

Thanks! I pushed a fix for the issue in 75805dce5ff8. @hans, would it be possible to check if D92808 + 75805dce5ff8 fixes the issue or should we just recommit D92808?

Yes, I confirmed that 75805dce5ff8 fixes the test case I was looking at. Thanks!

Thanks for confirming!

[ahatanak]

Thanks! Reapplied the patch in https://github.com/llvm/llvm-project/commit/1900503595cbb84a4c6e140a9ba1a2c574c0586d