Skip to content

Only enable cross-module inlining when explicitly set on the commandline.#1737

Merged
dnadlinger merged 1 commit intoldc-developers:masterfrom
JohanEngelen:disablecross
Sep 8, 2016
Merged

Only enable cross-module inlining when explicitly set on the commandline.#1737
dnadlinger merged 1 commit intoldc-developers:masterfrom
JohanEngelen:disablecross

Conversation

@JohanEngelen
Copy link
Member

A few subtle cross-module inlining bugs have been discovered, so let's disable it for now per default. See #1718.

This PR completely disables cross-module inlining unless explicitly enabled through the cmdline flag -enable-cross-module-inlining.
In particular, this disables cross-module inlining of pragma(inline, true) functions.

…ine.

In particular, this disables cross-module inlining of `pragma(inline, true)` functions.
@JohanEngelen JohanEngelen added this to the 1.1.0 milestone Sep 4, 2016
@JohanEngelen
Copy link
Member Author

(once merged into release-1.1.0, this will also need a release notes update)

@JohanEngelen JohanEngelen mentioned this pull request Sep 4, 2016
@JohanEngelen
Copy link
Member Author

I'd like to enable cross-module inlining at -O4, but it's up for debate. I think it's nice to be able to compile with -O4 such that compilation also works with earlier LDC versions (compared with the previously unknown -enable-cross-module-inlining) and get the boost of cross-module inlining when compiler with the new LDC. (e.g. in our own CMake file, I imagine using -O4 when LDC is detected)

Upon 1.1.0 release, I'd like to enable cross-module inlining per default again in master, so it gets more testing exposure.

@kinke
Copy link
Member

kinke commented Sep 4, 2016

I'd like to enable cross-module inlining at -O4

I'd prefer that over a new, temporary cmdline switch too. For people really already using optimization levels > 3 explicitly, I think a big fat note in the change log/announcements should be enough. I for one would surely check out these notes if I were confronted with new linking errors after upgrading LDC.

@dnadlinger dnadlinger merged commit d8249bb into ldc-developers:master Sep 8, 2016
@dnadlinger
Copy link
Member

I'd still greatly prefer if we could ship cross-module inlining by disabling it in the few cases that are known to be problematic (such as __FILE__ template parameters, …). Trying to paper over the cracks as in #1718 certainly isn't the way to go, though: Additional inlining always needs to be an "as-if" optimisation. You need to be able to stick if (rand() & 1) return false; into defineAsExternallyAvailable() and still have everything work as expected. Once that is the case, we have a full implementation of cross-module inlining and can ship it as we ship every other feature – just as for everything else, the added complexity intrinsically comes with a chance for bugs, but that can be mitigated by diligent review and pre-release testing.

@thewilsonator
Copy link
Contributor

DMDFE 2.079 has enabled [default parameters following varadics]https://dlang.org/changelog/2.079.0.html#default_after_variadic), is there much reason to have __FILE__ as a template parameter anymore (other than legacy)? and does that plug enough of the holes in cross-module inlining to reconsider enabling it by default?

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.

4 participants