LGTM, but you can wait for the final approval from @aprantl.

I agree with you. If it's not going to work, maybe there should have been a warning.

I see a warning when I compile the following (contrived) example:

In D94597#2632887, @fhahn wrote:

In D94597#2632806, @ab wrote:

I read through the discussion in D92808 but it sounds like that predates the x86_64-specific behavior, right? But skimming around the various passes involved I'm not actually sure who emits the retainARV/claimARV calls on aarch64.

On AArch64 I think that happens during the lowering of the ARC intrinsics pre-ISel, but @ahatanak would know more about the exact place.

Moved the header back in dca5737945b9a52d0c66c7ec0cbaa2e15ada69cf

I'll move the header back to Analysis.

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

Emit a call to @llvm.objc.retain instead of a call to @llvm.objc.retainAutoreleasedReturnValue.

On second thought, I think we should emit a call to @llvm.objc.retain instead of a call to @llvm.objc.retainAutoreleasedReturnValue. I think we might want to remove ARC optimizer's code that converts calls to @llvm.objc.retainAutoreleasedReturnValue to calls to @llvm.objc.retain since it sometimes does the transformation when it shouldn't and we won't need that transformation once we switch to using the clang.arc.attachedcall operand bundle.

We could emit a call to @llvm.objc.retain instead of the call to @llvm.objc.retainAutoreleasedReturnValue, but this doesn't seem to happen often and ARC optimizer can remove or replace the call anyway, so I'm just letting the front-end emit @llvm.objc.retainAutoreleasedReturnValue for simplicity.

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.

I managed to reproduce the failure by building everything with -DLLVM_APPEND_VC_REV=NO.

OK, I se the other patch is using the bit.

VarDecl has a bit (EscapingByref) that indicates whether the __block variable is captured by an escaping block. Can that information be used to avoid pre-moving to the heap?

I still don't understand why the test was failing, but did we have to bump the version because a new LangOpt (EncodeCXXClassTemplateSpec) was added?

In D96816#2572218, @thakis wrote:

Very cool change, I wanted something like this for a long time :)

It seems to break Index/preamble-reparse-changed-module.m on some platforms:
http://45.33.8.238/linux/39727/step_7.txt
http://45.33.8.238/macm1/3788/step_6.txt

Does the failure make sense to you?

• Use BoolFOption to define both the positive and negative options.
• Enable the optimization if the ObjC runtime is a NeXT-family runtime.

Add test cases for nested class.

Upload patch with context.

Enable this optimization only on Darwin.

In D96832#2570009, @zequanwu wrote:

In D96832#2569987, @ahatanak wrote:

In D96832#2569481, @zequanwu wrote:

In D96832#2568257, @aaron.ballman wrote:

It was explicitly disallowed in the initial patch: https://reviews.llvm.org/D12922 and the original author said "I'm still trying to figure out the best way to handle c++ virtual functions: this attribute is not very useful for someone who is looking for a way to reliably prevent tail-call to a virtual function." and "I made this change because this attribute isn't useful when the compiler cannot resolve the function call statically at compile time and it isn't important for the use case I have." Has this situation changed in the backend?

Oh, I didn't see that. But when I tested not_tail_called on normal functions, it seems like not working(https://godbolt.org/z/znr5b5, f1 is marked as not_tail_called, it still get inlined). Or, I misunderstand how to use it properly?

I'm still trying to remember the discussions we had, but I think we banned the attribute on virtual functions because you can't in general promise a call to an annotated function won't be tail called if the function is virtual.

This patch doesn't prevent the call to method in the code below from being tail called, but I suppose users would except the attribute to prevent the tail call?

struct B {
  virtual void method();  
};

struct D : B {
  [[clang::not_tail_called]] void method() override; 
};

void test(D *d) {
  B *b = D;
  b->method();
}

I got the point about why preventing it on virtual functions. But, on the above godbolt link, I provided a example where not_tail_called doesn't prevent a non-virtual function from tail call. I wonder if I use it correctly.

In D96832#2569481, @zequanwu wrote:

In D96832#2568257, @aaron.ballman wrote:

It was explicitly disallowed in the initial patch: https://reviews.llvm.org/D12922 and the original author said "I'm still trying to figure out the best way to handle c++ virtual functions: this attribute is not very useful for someone who is looking for a way to reliably prevent tail-call to a virtual function." and "I made this change because this attribute isn't useful when the compiler cannot resolve the function call statically at compile time and it isn't important for the use case I have." Has this situation changed in the backend?

Oh, I didn't see that. But when I tested not_tail_called on normal functions, it seems like not working(https://godbolt.org/z/znr5b5, f1 is marked as not_tail_called, it still get inlined). Or, I misunderstand how to use it properly?

This patch focuses only on reducing the encoded size of classes with template specialization, but if the information about pointed-to types isn't used at all, clang can always emit "^v" for C++ pointer types. Or it might make more sense to base the decision on whether the type is POD rather than on whether the string includes template specialization.

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

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

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.

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

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.

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

Fix two bugs which were discovered.

This patch causes opt to crash when the following IR is compiled:

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.

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.

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.

Make the alignment 16 bytes on 32-bit systems too.

Add a description of the new bundle to LangRef.

Address review comments.

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.

Address review comments.

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 pair if the call isn't marked as tail.

Address post-commit review comments.

Stop using clang.arc.rv_marker and just use "clang.arc.rv"="retain/claim".

Add a test for invoke and a FIXME for eliminating the need for emitARCOperationAfterCall.

Update patch.

Add helper functions to ObjCARCRVAttr.h for adding, removing, or looking up attributes.

ping

Rebase and fix a few bugs in the patch.

OK, I see.

LGTM. ARC contract pass isn't being run by the NPM, but you are going to change that in the future, I guess?

Is this change safe considering calls to EP.get() can add function declarations to the module as you mentioned in https://reviews.llvm.org/D86178?

Make isParamDestroyedInCallee a method of ParmVarDecl.

We could probably do something like what this patch is doing and determine whether a class can be passed in registers based on whether its subobjects can be passed in registers. If all of the subobjects can be passed in registers, the current class can be passed in registers too unless something declared in the current class forces it to be passed indirectly (e.g., a virtual function is declared).

This latest patch changes the way D is passed in the following example, which doesn't use trivial_abi at all:

Set the insert point before generating new instructions in insertRetainOrClaimRVCalls.

Rebase.

Rebase and ping.

Elaborate on when createCallInstWithColors should be called.

ToT clang/llvm (without this patch) is turning the call to @objc_unsafeClaimAutoreleasedReturnValue in the EH cleanup block into an unreachable when compiling the following code. It looks like this happens because the inliner isn't adding the funclet token, but it's not clear to me whether this is a bug or not.

Enable the optimization on Windows.

Hmm, it seems that calls to some of the ARC runtime functions don't get the token. In some cases, I'm seeing WinEHPrepare (ToT clang, without this patch) turn a call to @objc_unsafeClaimAutoreleasedReturnValue in an EH cleanup block into an unreachable.