DI Generation Improvements. #945

Closed
wants to merge 17 commits into
from

Conversation

Projects
None yet
7 participants
@LightBender
Contributor

LightBender commented May 13, 2012

This pull request improves the follow DI generation rules:

All functions stripped of their implementations except for template functions and auto return functions.
Improved indenting.

Requires: D-Programming-Language/druntime#218
Supersedes: D-Programming-Language#538

Fix issue: http://d.puremagic.com/issues/show_bug.cgi?id=1427
Fix issue: http://d.puremagic.com/issues/show_bug.cgi?id=5461

@andralex

This comment has been minimized.

Show comment
Hide comment
@andralex

andralex Sep 25, 2012

Member

@LightBender time to rebase and see what happens with the tester failures!

Member

andralex commented Sep 25, 2012

@LightBender time to rebase and see what happens with the tester failures!

@WalterBright

This comment has been minimized.

Show comment
Hide comment
@WalterBright

WalterBright Nov 11, 2012

Member

By stripping all the function bodies out, then functions in .di files lose the ability to be inlined. I disagree with this change.

I am also not seeing much point in pretty-printing .di files. If someone wants to build a D source code formatting filter, that would be the place for it. Then, the .di file could be piped through that. Otherwise, we'll be building a pretty-printer twice.

Member

WalterBright commented Nov 11, 2012

By stripping all the function bodies out, then functions in .di files lose the ability to be inlined. I disagree with this change.

I am also not seeing much point in pretty-printing .di files. If someone wants to build a D source code formatting filter, that would be the place for it. Then, the .di file could be piped through that. Otherwise, we'll be building a pretty-printer twice.

@LightBender

This comment has been minimized.

Show comment
Hide comment
@LightBender

LightBender Nov 11, 2012

Contributor

You realize that without this change D is pretty much useless for commercial library development as ALL function implementations are included in the DI file. It would be like writing your entire library in a header file. It doesn't matter if you're an OSS guy, but legal teams at commercial shops everywhere are going to scream bloody murder...

Don't expect any commercial libraries to be built for D without this.

Contributor

LightBender commented Nov 11, 2012

You realize that without this change D is pretty much useless for commercial library development as ALL function implementations are included in the DI file. It would be like writing your entire library in a header file. It doesn't matter if you're an OSS guy, but legal teams at commercial shops everywhere are going to scream bloody murder...

Don't expect any commercial libraries to be built for D without this.

@WalterBright

This comment has been minimized.

Show comment
Hide comment
@WalterBright

WalterBright Nov 11, 2012

Member

On 11/10/2012 7:28 PM, LightBender wrote:

You realize that without this change D is pretty much useless for commercial
library development as ALL function implementations are included in the DI file.

Just the ones that look like they are inlineable. Functions that have loops in
them are all not considered inlineable.

It would be like writing your entire library in a header file. It doesn't
matter if you're an OSS guy, but legal teams at commercial shops everywhere
are going to scream bloody murder...

Don't expect any commercial libraries to be built for D without this.

These days, an increasing amount of the code is winding up as templates anyway,
which simply must be included. But, that said, I understand the desire to hide
the function bodies. To that end, there are still possibilities:

  1. delete them manually. Ok, it's tedious, but it's not more work than it is for
    C++.
  2. use PIMPL (pointer to implementation) style, where all those .di functions do
    is transfer control elsewhere to hidden functions.

And, if someone is building a commercial library and considers themselves
blocked by this, please contact me.

Member

WalterBright commented Nov 11, 2012

On 11/10/2012 7:28 PM, LightBender wrote:

You realize that without this change D is pretty much useless for commercial
library development as ALL function implementations are included in the DI file.

Just the ones that look like they are inlineable. Functions that have loops in
them are all not considered inlineable.

It would be like writing your entire library in a header file. It doesn't
matter if you're an OSS guy, but legal teams at commercial shops everywhere
are going to scream bloody murder...

Don't expect any commercial libraries to be built for D without this.

These days, an increasing amount of the code is winding up as templates anyway,
which simply must be included. But, that said, I understand the desire to hide
the function bodies. To that end, there are still possibilities:

  1. delete them manually. Ok, it's tedious, but it's not more work than it is for
    C++.
  2. use PIMPL (pointer to implementation) style, where all those .di functions do
    is transfer control elsewhere to hidden functions.

And, if someone is building a commercial library and considers themselves
blocked by this, please contact me.

@andralex

This comment has been minimized.

Show comment
Hide comment
@andralex

andralex Nov 12, 2012

Member

I discussed this with Walter. There are a few reasonable views on this:

  1. Separate compilation is what it is, so do as Adam suggests.
  2. For the most part short, inlineable functions are unlikely to change often and efficiency is what it is, so keep inlineable function bodies in the .di file.
  3. Give people a possibility to direct inlining. A possibility raised by Walter is to force inlined functions to be templates by simply adding a "()" prior to the parameter list. That would put them in the .di file without otherwise affecting things (there are, however, some oddities such as overload resolution that I suspect will create puzzlement down the road). One other possibility that I just figured is to put in the .di by definition all functions that return "auto". I like this a whole lot better. A third possibility is to add a new attribute such as @alwaysinline or pragma(inline) to control inlining. Walter and I prefer working with the features we have instead of adding new ones. My vote goes to use of auto returns to control inlining.

Regarding the exchange: (a) please let's not overstate the case; (b) using C++ as a baseline is inadequate; (c) "if someone is building a commercial library and considers themselves blocked by this, please contact me" is rather provincial and non-scalable - it would behoove us to never talk or think that way.

Member

andralex commented Nov 12, 2012

I discussed this with Walter. There are a few reasonable views on this:

  1. Separate compilation is what it is, so do as Adam suggests.
  2. For the most part short, inlineable functions are unlikely to change often and efficiency is what it is, so keep inlineable function bodies in the .di file.
  3. Give people a possibility to direct inlining. A possibility raised by Walter is to force inlined functions to be templates by simply adding a "()" prior to the parameter list. That would put them in the .di file without otherwise affecting things (there are, however, some oddities such as overload resolution that I suspect will create puzzlement down the road). One other possibility that I just figured is to put in the .di by definition all functions that return "auto". I like this a whole lot better. A third possibility is to add a new attribute such as @alwaysinline or pragma(inline) to control inlining. Walter and I prefer working with the features we have instead of adding new ones. My vote goes to use of auto returns to control inlining.

Regarding the exchange: (a) please let's not overstate the case; (b) using C++ as a baseline is inadequate; (c) "if someone is building a commercial library and considers themselves blocked by this, please contact me" is rather provincial and non-scalable - it would behoove us to never talk or think that way.

@alexrp

This comment has been minimized.

Show comment
Hide comment
@alexrp

alexrp Nov 12, 2012

Member

@andralex @alwaysinline has been requested countless times by people on the newsgroup because they actually know what they're doing, but it keeps being either ignored or dismissed as unnecessary, which I think is absolute nonsense for a systems language. Most of these people now resort to compiler-specific attributes that GDC and LDC provide. This is a pretty silly situation when all compilers can relatively easily support it.

So, you know, there's that. Adding that attribute would kill two birds with one stone.

Member

alexrp commented Nov 12, 2012

@andralex @alwaysinline has been requested countless times by people on the newsgroup because they actually know what they're doing, but it keeps being either ignored or dismissed as unnecessary, which I think is absolute nonsense for a systems language. Most of these people now resort to compiler-specific attributes that GDC and LDC provide. This is a pretty silly situation when all compilers can relatively easily support it.

So, you know, there's that. Adding that attribute would kill two birds with one stone.

@andralex

This comment has been minimized.

Show comment
Hide comment
@andralex

andralex Nov 12, 2012

Member

Again, I'm favoring using the current language over adding new features.

Member

andralex commented Nov 12, 2012

Again, I'm favoring using the current language over adding new features.

@WalterBright

This comment has been minimized.

Show comment
Hide comment
@WalterBright

WalterBright Nov 12, 2012

Member

How about this:

If the switch -inline is used for the .di generation, then the bodies are included. Otherwise, not.

Member

WalterBright commented Nov 12, 2012

How about this:

If the switch -inline is used for the .di generation, then the bodies are included. Otherwise, not.

@andralex

This comment has been minimized.

Show comment
Hide comment
@andralex

andralex Nov 12, 2012

Member

Like

Member

andralex commented Nov 12, 2012

Like

@klickverbot

This comment has been minimized.

Show comment
Hide comment
@klickverbot

klickverbot Nov 13, 2012

Member

@andralex: Dislike. ;)

In my opinion, there is no good reason from a user's perspective why -inline should influence .di generation, as it is purely a code generation flag. Whether I use -inline to build my library is not related to whether I want function bodies to be kept when generating the import modules. As a litmus test, think about how you would change the description of the -inline flag if this behavior was implemented: »do function inlining; also keep function bodies on 'header' generation«?

I think this makes it clear that the switches have nothing to do with each other. There is nothing wrong in adding an additional command line flag for the .di generator (-Hkeep-function-bodies or whatever cryptic name instead), if, well, a toggle-able feature is added to the .di generator. If you are worried about the perceived complexity of the command line interface, add a separate »header generation« subsection to the help and group all the switches there. DMD certainly doesn't become less complex from a user's perspective when command line switches are arbitrarily overloaded with different meanings.

Member

klickverbot commented Nov 13, 2012

@andralex: Dislike. ;)

In my opinion, there is no good reason from a user's perspective why -inline should influence .di generation, as it is purely a code generation flag. Whether I use -inline to build my library is not related to whether I want function bodies to be kept when generating the import modules. As a litmus test, think about how you would change the description of the -inline flag if this behavior was implemented: »do function inlining; also keep function bodies on 'header' generation«?

I think this makes it clear that the switches have nothing to do with each other. There is nothing wrong in adding an additional command line flag for the .di generator (-Hkeep-function-bodies or whatever cryptic name instead), if, well, a toggle-able feature is added to the .di generator. If you are worried about the perceived complexity of the command line interface, add a separate »header generation« subsection to the help and group all the switches there. DMD certainly doesn't become less complex from a user's perspective when command line switches are arbitrarily overloaded with different meanings.

@andralex

This comment has been minimized.

Show comment
Hide comment
@andralex

andralex Nov 13, 2012

Member

I think it's very fit to have -inline mean keep inlined function bodies when generating .di files. Yes, it is a different meaning, but it's right on the spot. I don't see why people should remember a different flag when it comes about the same concept, just applied to .di generation.

Member

andralex commented Nov 13, 2012

I think it's very fit to have -inline mean keep inlined function bodies when generating .di files. Yes, it is a different meaning, but it's right on the spot. I don't see why people should remember a different flag when it comes about the same concept, just applied to .di generation.

@klickverbot

This comment has been minimized.

Show comment
Hide comment
@klickverbot

klickverbot Nov 13, 2012

Member

How is »do function inlining« related to »keep function bodies«? How are two meanings for the same flag easier to remember than two well-named ones? If you knew nothing about D, would dmd -H -inline <…> or dmd -H -keep-function-bodies <…> rather lead you to a correct guess of the intent of the invocation?

Member

klickverbot commented Nov 13, 2012

How is »do function inlining« related to »keep function bodies«? How are two meanings for the same flag easier to remember than two well-named ones? If you knew nothing about D, would dmd -H -inline <…> or dmd -H -keep-function-bodies <…> rather lead you to a correct guess of the intent of the invocation?

@WalterBright

This comment has been minimized.

Show comment
Hide comment
@WalterBright

WalterBright Nov 13, 2012

Member
  1. The only reason to keep the function bodies is to enable inlining of them.
  2. .di generation is done separately from code generation, so it is reasonable that the -inline flag pertain to it
  3. -di -inline means "I want to generate .di files and support inlining of functions in it". It sounds straightforward.
Member

WalterBright commented Nov 13, 2012

  1. The only reason to keep the function bodies is to enable inlining of them.
  2. .di generation is done separately from code generation, so it is reasonable that the -inline flag pertain to it
  3. -di -inline means "I want to generate .di files and support inlining of functions in it". It sounds straightforward.
@klickverbot

This comment has been minimized.

Show comment
Hide comment
@klickverbot

klickverbot Nov 13, 2012

Member

@andralex: Regarding the previous question, I think it is a proven fact that being able to override the compiler's inline heuristics is a useful feature in certain, rare cases. One of these is certainly real-time graphics/game engines, where you often can't afford a function call to be inserted for math code, even in debug mode. For example, Manu/@TurkeyMan has asked several times for something like this on the newsgroup.

Note that this has nothing to do with the C++ inline attribute which is indeed more or less an exercise in futility. pragma(alwaysInline); (which is my preferred syntax), just like the vendor-specific forceinline/always_inline attributes in C compilers, is a sharp tool for solving specific performance problems, which would be hard to attack otherwise. It is not intended to be used frequently, and you can easily shoot yourself in the foot performance-wise if you apply it without proper benchmarking. It's a compiler pragma, not a language feature, for a reason.

However, contrary to @alexrp, I don't think this should have anything to do with the problem discussed here, other than the fact that the .di generator could keep function bodies which are marked pragma(alwaysInline). The simple reason for this is that for the vast majority of cases where inlining entails a performance bit, alwaysInline will not be used, by design.

Member

klickverbot commented Nov 13, 2012

@andralex: Regarding the previous question, I think it is a proven fact that being able to override the compiler's inline heuristics is a useful feature in certain, rare cases. One of these is certainly real-time graphics/game engines, where you often can't afford a function call to be inserted for math code, even in debug mode. For example, Manu/@TurkeyMan has asked several times for something like this on the newsgroup.

Note that this has nothing to do with the C++ inline attribute which is indeed more or less an exercise in futility. pragma(alwaysInline); (which is my preferred syntax), just like the vendor-specific forceinline/always_inline attributes in C compilers, is a sharp tool for solving specific performance problems, which would be hard to attack otherwise. It is not intended to be used frequently, and you can easily shoot yourself in the foot performance-wise if you apply it without proper benchmarking. It's a compiler pragma, not a language feature, for a reason.

However, contrary to @alexrp, I don't think this should have anything to do with the problem discussed here, other than the fact that the .di generator could keep function bodies which are marked pragma(alwaysInline). The simple reason for this is that for the vast majority of cases where inlining entails a performance bit, alwaysInline will not be used, by design.

@andralex

This comment has been minimized.

Show comment
Hide comment
@andralex

andralex Nov 13, 2012

Member

I'm totally with Walter on 1-3, he essentially spelled it for me. @klickverbot you're making a good point about forcing inline but let's leave that to a future diff. @LightBender could you please do the honors of implementing the decision depending on -inline? Thanks all!

Member

andralex commented Nov 13, 2012

I'm totally with Walter on 1-3, he essentially spelled it for me. @klickverbot you're making a good point about forcing inline but let's leave that to a future diff. @LightBender could you please do the honors of implementing the decision depending on -inline? Thanks all!

@klickverbot

This comment has been minimized.

Show comment
Hide comment
@klickverbot

klickverbot Nov 13, 2012

Member

@WalterBright: What do you mean by ».di generation is done separately from code generation«? If you mean on a compiler level, then obviously yes, I think I'm somewhat familiar with that. If you mean that it is done separately as in not in the same compiler invocation in which code is generated, then that's not always the case. Apparently quite a few people use -H along with compilation (I discovered that when investigating an LDC bug), which actually makes sense when building/installing open source packages (and for whatever reason you assume that it is beneficial to install .di files, which is questionable).

The fundamental problem I have with the idea is that -inline seems to mean »do something now«, whereas keeping the function bodies around enables something else to be done later. Why doesn't -di -inline mean »I want to generate .di files and have inlining of functions performed«?

I still think there is no good reason to conflate the two concepts. You probably wouldn't expect -finline-functions to keep around function bodies either if GCC supported an analogous feature. I for sure wouldn't. If you think this makes the compiler simpler to understand, then apparently our definitions of »simple« differ…

Member

klickverbot commented Nov 13, 2012

@WalterBright: What do you mean by ».di generation is done separately from code generation«? If you mean on a compiler level, then obviously yes, I think I'm somewhat familiar with that. If you mean that it is done separately as in not in the same compiler invocation in which code is generated, then that's not always the case. Apparently quite a few people use -H along with compilation (I discovered that when investigating an LDC bug), which actually makes sense when building/installing open source packages (and for whatever reason you assume that it is beneficial to install .di files, which is questionable).

The fundamental problem I have with the idea is that -inline seems to mean »do something now«, whereas keeping the function bodies around enables something else to be done later. Why doesn't -di -inline mean »I want to generate .di files and have inlining of functions performed«?

I still think there is no good reason to conflate the two concepts. You probably wouldn't expect -finline-functions to keep around function bodies either if GCC supported an analogous feature. I for sure wouldn't. If you think this makes the compiler simpler to understand, then apparently our definitions of »simple« differ…

@WalterBright

This comment has been minimized.

Show comment
Hide comment
@WalterBright

WalterBright Nov 13, 2012

Member

@klickverbot, you're right, Manu's feature request is distinct from this one. "alwaysinline" is not the negation of "neverinline".

Member

WalterBright commented Nov 13, 2012

@klickverbot, you're right, Manu's feature request is distinct from this one. "alwaysinline" is not the negation of "neverinline".

@alexrp

This comment has been minimized.

Show comment
Hide comment
@alexrp

alexrp Nov 13, 2012

Member

The only reason to keep the function bodies is to enable inlining of them.

... and CTFE.

Member

alexrp commented Nov 13, 2012

The only reason to keep the function bodies is to enable inlining of them.

... and CTFE.

@TurkeyMan

This comment has been minimized.

Show comment
Hide comment
@TurkeyMan

TurkeyMan Nov 13, 2012

Contributor

... and CTFE.

And this is rather a big deal.

Contributor

TurkeyMan commented Nov 13, 2012

... and CTFE.

And this is rather a big deal.

@klickverbot

This comment has been minimized.

Show comment
Hide comment
@klickverbot

klickverbot Nov 13, 2012

Member

Hm, regarding CTFE, wouldn't .di generation simply not be used at all in that case?

Also, please note that while I still don't see why merging the additional flag into -inline would be beneficial, I don't think the issue is important enough to warrant lengthy discussion. So, if others agree, please go ahead and implement it like that. Originally, I only intended to make sure that the (non-)relation between alwaysInline and the discussed topic is clarified.

Member

klickverbot commented Nov 13, 2012

Hm, regarding CTFE, wouldn't .di generation simply not be used at all in that case?

Also, please note that while I still don't see why merging the additional flag into -inline would be beneficial, I don't think the issue is important enough to warrant lengthy discussion. So, if others agree, please go ahead and implement it like that. Originally, I only intended to make sure that the (non-)relation between alwaysInline and the discussed topic is clarified.

@LightBender

This comment has been minimized.

Show comment
Hide comment
@LightBender

LightBender Jan 9, 2013

Contributor

I have added the -inline switch support, new unittests for DMD covering the new -inline switch, and cleaned up some dead code that was used in earlier iterations of this pull request.

This is now ready to merge.

I want to give a shout-out to @AndrejMitrovic for helping me out with the Unittests.

Contributor

LightBender commented Jan 9, 2013

I have added the -inline switch support, new unittests for DMD covering the new -inline switch, and cleaned up some dead code that was used in earlier iterations of this pull request.

This is now ready to merge.

I want to give a shout-out to @AndrejMitrovic for helping me out with the Unittests.

@WalterBright

View changes

src/root/root.c
@@ -1545,10 +1545,18 @@ void OutBuffer::prependstring(const char *string)
void OutBuffer::writenl()
{
#if _WIN32
+#if M_UNICODE

This comment has been minimized.

@WalterBright

WalterBright Jan 9, 2013

Member

There's no reason to support M_UNICODE. None of the rest of the code does.

@WalterBright

WalterBright Jan 9, 2013

Member

There's no reason to support M_UNICODE. None of the rest of the code does.

This comment has been minimized.

@LightBender

LightBender Jan 9, 2013

Contributor

Ok, i'll remove it. I think that was a hold-over from when I copied kenji's old pretty printing pull because I didn't write that.

@LightBender

LightBender Jan 9, 2013

Contributor

Ok, i'll remove it. I think that was a hold-over from when I copied kenji's old pretty printing pull because I didn't write that.

@@ -23,6 +23,8 @@ struct HdrGenState
int inBinExp;
int inArrExp;
int emitInst;
+ int autoMember; // Non-zero if function is an auto type

This comment has been minimized.

@WalterBright

WalterBright Jan 9, 2013

Member

autoMember and tpltMember both suffer from the fault of being transitive - they are only meant to apply to one level. Nested functions will wrongly "inherit" these flags.

@WalterBright

WalterBright Jan 9, 2013

Member

autoMember and tpltMember both suffer from the fault of being transitive - they are only meant to apply to one level. Nested functions will wrongly "inherit" these flags.

This comment has been minimized.

@LightBender

LightBender Jan 9, 2013

Contributor

My usage of autoMember is pretty safe, the only place it's used anywhere is lines 1702 & 1704 of func.c. And all it does is tell the bodyToCBuffer function if it's an auto function.

As for tpltMember, do you have any suggestions on how to go about communicating whether or not it's inside a template function?

@LightBender

LightBender Jan 9, 2013

Contributor

My usage of autoMember is pretty safe, the only place it's used anywhere is lines 1702 & 1704 of func.c. And all it does is tell the bodyToCBuffer function if it's an auto function.

As for tpltMember, do you have any suggestions on how to go about communicating whether or not it's inside a template function?

@WalterBright

View changes

src/func.c
- if (fbody &&
- (!hgs->hdrgen || hgs->tpltMember || canInline(1,1,1))
- )
+ if (fbody && (!hgs->hdrgen || global.params.useInline || hgs->autoMember || hgs->tpltMember))
{ buf->writenl();

This comment has been minimized.

@WalterBright

WalterBright Jan 9, 2013

Member

In here:

  1. save values of autoMember and tpltMember
  2. set autoMember and tpltMember to 0
  3. execute code in { }
  4. restore autoMember and tpltMember to their saved values
@WalterBright

WalterBright Jan 9, 2013

Member

In here:

  1. save values of autoMember and tpltMember
  2. set autoMember and tpltMember to 0
  3. execute code in { }
  4. restore autoMember and tpltMember to their saved values
+ int saveauto = hgs->autoMember;
+ hgs->tpltMember = 0;
+ hgs->autoMember = 0;
+

This comment has been minimized.

@LightBender

LightBender Jan 10, 2013

Contributor

@WalterBright Is this what you had in mind?

@LightBender

LightBender Jan 10, 2013

Contributor

@WalterBright Is this what you had in mind?

This comment has been minimized.

@WalterBright

WalterBright Jan 10, 2013

Member

yes, that works

@WalterBright

WalterBright Jan 10, 2013

Member

yes, that works

@LightBender

This comment has been minimized.

Show comment
Hide comment
@LightBender

LightBender Jan 15, 2013

Contributor

Due to some irretrievable merge conflicts in the last rebase I have had to close this pull and start again. See pull: D-Programming-Language#1487 for the new pull request.

Contributor

LightBender commented Jan 15, 2013

Due to some irretrievable merge conflicts in the last rebase I have had to close this pull and start again. See pull: D-Programming-Language#1487 for the new pull request.

@LightBender LightBender deleted the LightBender:digen branch Jan 15, 2013

klickverbot added a commit to klickverbot/dmd that referenced this pull request May 6, 2016

Split global.params.hdrStripPlainFunctions from useInline
As discussed in the original PR that added the feature (#945), DMD currently
triggers two entirely unrelated features with the -inline switch. Since LDC
does not really have an equivalent to that switch in the first place, but
users still want to control the emission of function bodies, we need to be
control it separately.

For DMD, global.params.hdrStripPlainFunctions is always set to
!global.params.useInline in the driver to match the current behavior.

klickverbot added a commit to klickverbot/dmd that referenced this pull request May 6, 2016

Split global.params.hdrStripPlainFunctions from useInline
As discussed in the original PR that added the feature (#945), DMD currently
triggers two entirely unrelated features with the -inline switch. Since LDC
does not really have an equivalent to that switch in the first place, but
users still want to control the emission of function bodies, we need to be
able to control it separately.

For DMD, global.params.hdrStripPlainFunctions is always set to
!global.params.useInline in the driver to match the current behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment