Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

DI Generation Improvements. #945

Closed
wants to merge 17 commits into from

7 participants

@LightBender

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: #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
Owner

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

@WalterBright

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

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
@andralex
Owner

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

@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
Owner

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

@WalterBright

How about this:

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

@andralex
Owner

Like

@klickverbot
Collaborator

@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
Owner

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
Collaborator

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
  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
Collaborator

@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
Owner

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
Collaborator

@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

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

@alexrp

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

... and CTFE.

@TurkeyMan

... and CTFE.

And this is rather a big deal.

@klickverbot
Collaborator

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

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.

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

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@WalterBright WalterBright commented on the diff
src/hdrgen.h
@@ -23,6 +23,8 @@ struct HdrGenState
int inBinExp;
int inArrExp;
int emitInst;
+ int autoMember; // Non-zero if function is an auto type
@WalterBright Owner

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.

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/func.c
@@ -1790,21 +1805,21 @@ int FuncDeclaration::equals(Object *o)
void FuncDeclaration::bodyToCBuffer(OutBuffer *buf, HdrGenState *hgs)
{
- if (fbody &&
- (!hgs->hdrgen || hgs->tpltMember || canInline(1,1,1))
- )
+ if (fbody && (!hgs->hdrgen || global.params.useInline || hgs->autoMember || hgs->tpltMember))
{ buf->writenl();
@WalterBright Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@LightBender LightBender commented on the diff
src/func.c
@@ -1790,21 +1805,27 @@ int FuncDeclaration::equals(Object *o)
void FuncDeclaration::bodyToCBuffer(OutBuffer *buf, HdrGenState *hgs)
{
- if (fbody &&
- (!hgs->hdrgen || hgs->tpltMember || canInline(1,1,1))
- )
- { buf->writenl();
+ if (fbody && (!hgs->hdrgen || global.params.useInline || hgs->autoMember || hgs->tpltMember))
+ {
+ int savetlpt = hgs->tpltMember;
+ int saveauto = hgs->autoMember;
+ hgs->tpltMember = 0;
+ hgs->autoMember = 0;
+

@WalterBright Is this what you had in mind?

@WalterBright Owner

yes, that works

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@LightBender

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

@LightBender LightBender deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 27, 2012
  1. @LightBender
  2. @LightBender
Commits on Dec 13, 2012
  1. @donc

    Merge pull request #1373 from 9rnsr/fix9150

    donc authored
    Issue 9150 - Mismatching static array length should be detected in foreach
Commits on Dec 14, 2012
  1. @LightBender
  2. @LightBender
  3. @LightBender
  4. @LightBender
Commits on Jan 9, 2013
  1. @LightBender
  2. @LightBender
  3. @LightBender
  4. @LightBender
  5. @LightBender

    Fixed a bug that would cause some functions to not receive their impl…

    LightBender authored
    …ementations with -inline specified. Added unittests for DMD for the -inline switch.
  6. @LightBender
  7. @LightBender
  8. @LightBender
Commits on Jan 10, 2013
  1. @LightBender
  2. @LightBender
Something went wrong with that request. Please try again.