This is an archive of the discontinued LLVM Phabricator instance.

Put code that avoids heapifying local blocks behind a flag
ClosedPublic

Authored by waltl on Aug 17 2021, 2:30 PM.

Details

Summary

This change puts the functionality in commit c5792aa90fa45a1842f190c146f19e2c71ea6fbd behind a flag that is off by default. The original commit is not in Apple's Clang fork (and blocks are an Apple extension in the first place), and there is one known issue that needs to be addressed before it can be enabled safely.

Diff Detail

Event Timeline

jyknight requested review of this revision.Aug 17 2021, 2:30 PM
jyknight created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2021, 2:30 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Ping. Any comment from Apple folks?

Sorry for the delay in responding.

Instead of reverting the commit, can we put this optimization behind an off-by-default flag? As far as we can tell, the only problem we have now that is preventing us to enable this optimization is that clang doesn't copy a block to the heap when the block is converted to a non-block type (e.g., const void *) in some cases.

For example, the block isn't copied to the heap in the following example. Note that this happens without the optimization this patch is reverting.

void foo1(const void *) __attribute__((cf_audited_transfer ));

void foo2(id a) {
  // clang should copy the block to the heap, since foo1 doesn't know it's receiving a block, but it doesn't.
  foo1((const void *)^{ (void)a; });
}

This doesn't look too hard to fix. We probably just have to insert ARCExtendBlockObject into the AST.

Once we fix that problem, we can turn on the flag in both open source and Apple's compiler.

waltl commandeered this revision.Sep 7 2021, 6:53 PM
waltl added a reviewer: jyknight.
waltl updated this revision to Diff 371467.Sep 8 2021, 3:59 PM

As suggested, I put the optimization behind an off-by-default flag.

Do you need arc-blocks-avoid-heapify.m? It seems like the other tests already cover all the cases we care about.

waltl updated this revision to Diff 372017.Sep 10 2021, 2:51 PM

Delete redundant tests in arc-blocks-avoid-heapify.m.

waltl added a comment.Sep 10 2021, 2:52 PM

Do you need arc-blocks-avoid-heapify.m? It seems like the other tests already cover all the cases we care about.

You're right much of it is redundant, but test10a() and test10b() are still meaningful. I've deleted the redundant tests.

waltl retitled this revision from Revert "Avoid needlessly copying a block to the heap when a block literal" to Put code that avoids heapifying local blocks behind a flag.Sep 10 2021, 2:54 PM
waltl edited the summary of this revision. (Show Details)
waltl updated this revision to Diff 372031.Sep 10 2021, 4:35 PM
waltl edited the summary of this revision. (Show Details)

Merge arc-blocks-avoid-heapify.m into arc-blocks.m

waltl updated this revision to Diff 372035.Sep 10 2021, 4:41 PM

Trivial cleanup.

ahatanak accepted this revision.Sep 10 2021, 5:00 PM

Can you add a driver test, which tests both -fobjc-avoid-heapify-local-blocks and -fno-objc-avoid-heapify-local-blocks?

Other than that, LGTM.

This revision is now accepted and ready to land.Sep 10 2021, 5:00 PM
waltl updated this revision to Diff 372056.Sep 10 2021, 10:27 PM

Added driver flags, and tests for them

Added driver flags, and tests for them

@ahatanak did you intend to ask Walt to add a driver flag for this?

I think we should not have one, since this isn't something we should be telling users to specify, or that we want to promise stability for.

I was just asking for test cases as I thought it was already a driver option, but it turns out it wasn't. I agree that this shouldn't be a driver option.

Sorry @waltl, please revert the changes that made it a driver option in the last patch.

waltl updated this revision to Diff 372306.Sep 13 2021, 11:46 AM

Remove driver flags and corresponding tests

I was just asking for test cases as I thought it was already a driver option, but it turns out it wasn't. I agree that this shouldn't be a driver option.

Sorry @waltl, please revert the changes that made it a driver option in the last patch.

Done. Thanks for the clarification.

jyknight accepted this revision.Sep 13 2021, 4:06 PM
jyknight added inline comments.
clang/include/clang/Driver/Options.td
2376

Needs to be marked [CC1Option, NoDriverOption]

waltl updated this revision to Diff 372383.Sep 13 2021, 7:07 PM

Mark new option as NoDriverOption

This revision was landed with ongoing or failed builds.Sep 14 2021, 11:06 AM
This revision was automatically updated to reflect the committed changes.