Skip to content

Conversation

@shajrawi
Copy link

@shajrawi shajrawi commented Dec 22, 2016

radar rdar://problem/26901382

Reduce the size of the text area (code size of the compiled output) by changing the way we generate LLVM IR for loadable enums:

For every retain / release (IRGen's copy / consume functions) we used to take the enum's explosion and create inlined copy / consume code.
Said code would do a LLVM switch for the enum's current element, creating Basic Blocks for each element that needs a retain / release...etc
The code above got re-generated each time we did a retain / release on that enum. potentially a large number of times.
This PR overcomes this issue by creating, for every loadable enum declaration, outlined function(s) that take an explosion of the loadable type and call retain/release for each appropriate element.
Copy / Consume methods just have to call said outlined function with the appropriate explosion.

One caveat for this implementation is that we don't always have debug information for said call-site (for example when compiling parts the standard library) - For now, I had to workaround this issue by disabling the assert that assumed there's debug information for each call that we create during IRGen.

Another tricky bit is the "naming" of the outlined functions:
We need new mangled names for the consume / copy, which I added in IRGen's Mangler, and added initial support for demangling / remangling of said name, but, the mangled name is not ideal -
We are at 80% capacity in our types, instead of creating a new type for this optimization we are re-using the 'W' prefix, followed by a letter that indicates if this is an outlined copy or outlined consume function.

A final caveat is that code-size reduction with this approach relies on frequent re-use of loadable enums that need retain / releases. small enums, that can be passed via registers, will benefit from this the most (we are not doing function calls that require pushes / pops from stack).

Measuring the code size reduction on Swift's code base, even though, due to the caveat above, it is not the ideal test-case to show large size reductions, shows some promise on (x86-64) release builds:
Benchmark_Onone __text: 787027 782915 0.5%
libswiftStdlibCollectionUnittest.dylib __text: 1747772 1699932 2.7%
libswiftStdlibUnittestFoundationExtras.dylib __text: 3226 3210 0.5%

I dived into Benchmark_Onone: there are only a few benchmarks that actually contain heavy use of Enums (mainly some of the dictionaries and sets ones) - their code size is down by over 20%

I haven't checked the size-reduction on debug version of the code base above (yet), even though the size reduction might be much higher.
I also haven't checked the size-reduction on ARM (yet), even though the size reduction might be much higher due to the larger number of general purpose registers.

@shajrawi
Copy link
Author

@eeckstein Can you please review the mangling / de-mangling / re-mangling?
@aschwaighofer Can you please review IRGen?

@shajrawi
Copy link
Author

@swift-ci Please smoke test

Copy link
Contributor

@eeckstein eeckstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same here: I think it's better to just say "outlined copy of "

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rename the nodes to OutlinedCopy/OutlinedConsume. The "enum" is already part of the type. We can then also reuse them for other types than enums.

@shajrawi shajrawi force-pushed the outline_enum_retain_release branch from fb17e0f to bcfec44 Compare December 22, 2016 19:03
@shajrawi
Copy link
Author

@eeckstein I Updated the PR with the changes you requested.

I still have two Items on my TODO list:

  1. Per @aschwaighofer offline review, do not outline the copy / consume when calling from witness functions - use the old code instead. This would require adding a new boolean (defaulted to false) throughout IRGen - similar to the atomicity flag
  2. Fix the test case that fails on Linux

@shajrawi shajrawi force-pushed the outline_enum_retain_release branch from bcfec44 to ee5ed7b Compare December 23, 2016 01:08
@shajrawi
Copy link
Author

Small update to PR - Fixed the test case that fails on Linux

@shajrawi
Copy link
Author

shajrawi commented Jan 4, 2017

After looking into copy / consume when calling from witness functions - as per my talk with @aschwaighofer, it seems that such calls are already inlined! we don't need to propagate a large "IsOutlined" change / boolean flag throughout IRGen: the implementation of GenEnum's assignWithCopy is different from the copy function / does not call it. It does, however, contain a slightly different (inlined) code-path - see emitIndirectAssign for details.

@shajrawi
Copy link
Author

shajrawi commented Jan 4, 2017

@swift-ci Please smoke test

@shajrawi shajrawi merged commit 66e4597 into swiftlang:master Jan 4, 2017
@shajrawi shajrawi deleted the outline_enum_retain_release branch January 9, 2017 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants