Issue 10985 - Compiler doesn't attempt to inline non-templated functions from libraries (even having the full source) #2561

9rnsr · 2013-09-16T06:25:43Z

http://d.puremagic.com/issues/show_bug.cgi?id=10985

This PR requires druntime fix: https://github.com/dlang/druntime/pull/608
Today, implicitly generated code for assertions/array-bounds-check uses ModuleInfo object.
However ModuleInfo object won't be generated if the module is not in root - even though the code in module could be directly used by template instantiation and/or inlining.

To fix the issues:

Stop using ModuleInfo for assertion and array bounds check.

Relax the condition for inlining, due to fix bug 10985.

~~During the work, I found a wrong-code [bug 11049](http://d.puremagic.com/issues/show_bug.cgi?id=11049), and this PR also fixes it.~~

ibuclaw · 2013-09-16T06:48:51Z

+1 on removing _d_array_boundsm, _d_assertm, _d_unittestm (should these be removed from druntime too?)

-1 on slowing down compilation speed. :-)

9rnsr · 2013-09-16T06:53:31Z

should these be removed from druntime too?

I'm worrying that immediately removing them might break backward compatibility.

ibuclaw · 2013-09-16T07:46:05Z

Ok, no problem. We don't yet ship phobos/druntime as a shared library, so I can't imagine how though.

MartinNowak · 2013-09-16T09:13:12Z

Great to finally address this issue, thanks a lot Kenji.

I'm worrying that immediately removing them might break backward compatibility.

Let's remove them after the next release.
I usually try to keep old stuff around for a while because it's annoying to match druntime and dmd versions.
Immediately removing runtime symbols requires to rebase local dmd branches which don't have related changes and makes bisecting much harder.

ibuclaw · 2013-09-16T09:42:01Z

Also +1 on code convergence (gdc has never implemented the toModuleArray methods... should get round to either supporting or removing _d_switch_errorm in the next release merge)

ibuclaw · 2013-09-16T17:48:11Z

Did Walter break the testsuite again?

ghost · 2013-09-16T21:15:01Z

@9rnsr: If the compiler can now do this analysis, could it also try and error on this code:

class C
{
    void foo() { }
    void foo() { }
}

void main() { }

This currently creates a linker error, but this should really be caught in the compiler.

9rnsr · 2013-09-17T09:07:24Z

Currently some issues there.

Out of memory error of runnable/test19.d in Win32
Fixed by removing import of std.string.
Link-failures by undefined symbols
It was the appearance of bug 9571.
Fixed by Issue 9571 - link error due to using unique ids in anonymous funcliteral #2566, and now this PR relies on that.
Unittest failure of std/numeric.d(1416) in Linux 64bit codegen
I'm still not sure the reason.
"corrupt or invalid COFF symbol table" error on creating druntime.lib in Win64
Still not sure the reason, too.

9rnsr · 2013-09-17T09:08:11Z

this should really be caught in the compiler

@AndrejMitrovic I agree. It should be fixed in the future.

ibuclaw · 2013-09-18T23:47:26Z

Currently some issues there.

I wish the best for fixing them in dmd and I'm looking forward to this. :-)

9rnsr · 2013-09-19T03:56:56Z

src/e2ir.c

+    }
+    else
+    {
+        efilename = m->toEfilename();


Generating module filename string and using the symbol causes LNK1235: corrupt or invalid COFF symbol table error on Win64 platform, but I cannot find the reason. (Maybe a bug around coff object file generation?)
@WalterBright Can you help me?

MartinNowak · 2013-09-19T10:01:58Z

Here is a reduced test case for the std.numeric failure.

bool normalize(double[] range, double sum = 1)
{
    double s = 0;
    // Step 1: Compute sum and length of the range
    const length = range.length;
    foreach (e; range)
    {
        s += e;
    }
    // Step 2: perform normalization
    if (s == 0)
    {
        return false;
    }
    // The path most traveled
    return true;
}

This generate wrong code for s == 0. It reuses rdx where it previously loaded the content of the xmm register from the stack. You can compare the old (at line 35) and the new (at line 32) dissassembly.

If I remove the SFLexit flag from rtlsym.h it works, this seems to have some side-effect on the backend register usage.

rainers · 2013-09-21T10:09:43Z

Generating module filename string and using the symbol causes LNK1235: corrupt or invalid COFF symbol table error on Win64 platform, but I cannot find the reason. (Maybe a bug around coff object file generation?)

Maybe related to http://d.puremagic.com/issues/show_bug.cgi?id=11081. I noticed that function literals sometimes don't have a name, I guess this happens if they are created during speculative template instantiations.

MartinNowak · 2013-10-15T18:30:33Z

Oh, I thought this was already merged. What issues still need a fix?

9rnsr · 2013-10-15T20:08:50Z

@dawgfoto Wrong-code bugs in some platforms is not yet fixed. Unfortunately I don't have enough time to debug it now.

MartinNowak · 2013-10-15T23:27:25Z

@dawgfoto Wrong-code bugs in some platforms is not yet fixed. Unfortunately I don't have enough time to debug it now.

No problem, I put it on my list, but I might not find much time for it either.

DmitryOlshansky · 2013-11-28T19:48:59Z

Pardon for nagging. Just a couple of questions:

What are the chances to get this in 2.065? I guess it may be disruptive.
Will this fix inlining of Phobos (and other libraries) for all compilers? Or is this specific fix for DMD inliner?

MartinNowak · 2013-11-28T21:44:26Z

What are the chances to get this in 2.065? I guess it may be disruptive.

It might trigger a lot of currently hidden inline/codegen bugs.

Safety0ff · 2014-03-18T20:17:06Z

The 64bit code gen errors seem to be the same ones triggered by PR #1426

ghost · 2014-03-18T20:36:26Z

The 64bit code gen errors seem to be the same ones triggered by PR #1426

I'm glad to hear this, I thought I must have been doing something really wrong but maybe we're both running into some code that accidentally triggers this bug..

9rnsr · 2014-05-16T09:01:53Z

Fix generation of __assert, __arrayZ, and __unittest_fail. Now they are correctly generated for instantiated templates, and anymore ModuleInfo is not necessary for them.

Ready to merge.

MartinNowak · 2014-05-16T21:16:23Z

src/glue.c

@@ -520,7 +531,7 @@ void Module::genobjfile(bool multiobj)
                elinnum = el_var(sp);
            }

-            elem *efilename = el_ptr(toSymbol(this));
+            elem *efilename = toEfilename(this);


Should we keep using the modulename instead of the filename?

I found that the issue 846 fix is unnecessary for this PR, so it is remove now.

Why aren't the missing assert functions a problem?
When I compile a library in release mode but do the inlining for a debug build, who is emitting the assert functions?

If you compile the release library with -lib switch, compiler will generate and store the assert functions in the library file. In other cases, you should link debug builded library for a debug build of your application.

As a note, now I'm separating issue 846 fix to #3552.

MartinNowak · 2014-05-16T21:17:37Z

LGTM.
Do you know what resolved the wrong-code issue?

This is important due to avoid out-of-memory error with `-unittest -inline`. Currently front-end inlining requires running semantic3 for all imported modules to analyze expanded function body code. By this change, we can remove the Phobos unittest analyzing cost in user application compilation.

…nctions from libraries (even having the full source)

9rnsr · 2014-05-17T06:57:46Z

Updated commits. Now this PR only fixes issue 10985.

MartinNowak · 2014-05-17T16:25:43Z

Auto-merge toggled on

Issue 10985 - Compiler doesn't attempt to inline non-templated functions from libraries (even having the full source)

WalterBright · 2014-06-04T09:41:19Z

@MartinNowak I reported your bug report as https://issues.dlang.org/show_bug.cgi?id=12852

WalterBright · 2014-06-05T17:59:58Z

Unittest failure of std/numeric.d(1416) in Linux 64bit codegen
I'm still not sure the reason.

#3624

WalterBright · 2014-07-27T22:42:08Z

This appears to have caused regression https://issues.dlang.org/show_bug.cgi?id=13193

9rnsr reviewed Sep 19, 2013
View reviewed changes

This was referenced Oct 13, 2013

Some simple std.regex optimization dlang/phobos#1553

Merged

make case tables immutable dlang/phobos#1636

Merged

9rnsr mentioned this pull request Jan 10, 2014

Issue 11881 - -betterC switch suffers from bit rot #3072

Closed

MartinNowak reviewed May 16, 2014
View reviewed changes

9rnsr added 2 commits May 17, 2014 15:35

fix Issue 10985 - Compiler doesn't attempt to inline non-templated fu…

9cf4601

…nctions from libraries (even having the full source)

9rnsr mentioned this pull request May 17, 2014

Issue 846 - Error 42: Symbol Undefined "<mangle_of_class_template>__arrayZ" #3552

Merged

MartinNowak added a commit that referenced this pull request May 17, 2014

Merge pull request #2561 from 9rnsr/fix10985

c2794ce

Issue 10985 - Compiler doesn't attempt to inline non-templated functions from libraries (even having the full source)

MartinNowak merged commit c2794ce into dlang:master May 17, 2014

9rnsr deleted the fix10985 branch May 18, 2014 05:03

Safety0ff mentioned this pull request Jun 4, 2014

add some array bounds check optimizations #3620

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 10985 - Compiler doesn't attempt to inline non-templated functions from libraries (even having the full source) #2561

Issue 10985 - Compiler doesn't attempt to inline non-templated functions from libraries (even having the full source) #2561

9rnsr commented Sep 16, 2013

ibuclaw commented Sep 16, 2013

9rnsr commented Sep 16, 2013

ibuclaw commented Sep 16, 2013

MartinNowak commented Sep 16, 2013

ibuclaw commented Sep 16, 2013

ibuclaw commented Sep 16, 2013

ghost commented Sep 16, 2013

9rnsr commented Sep 17, 2013

9rnsr commented Sep 17, 2013

ibuclaw commented Sep 18, 2013

9rnsr Sep 19, 2013

MartinNowak commented Sep 19, 2013

rainers commented Sep 21, 2013

MartinNowak commented Oct 15, 2013

9rnsr commented Oct 15, 2013

MartinNowak commented Oct 15, 2013

DmitryOlshansky commented Nov 28, 2013

MartinNowak commented Nov 28, 2013

Safety0ff commented Mar 18, 2014

ghost commented Mar 18, 2014

9rnsr commented May 16, 2014

MartinNowak May 16, 2014

9rnsr May 17, 2014

MartinNowak May 17, 2014

9rnsr May 17, 2014

9rnsr May 17, 2014

MartinNowak commented May 16, 2014

9rnsr commented May 17, 2014

MartinNowak commented May 17, 2014

WalterBright commented Jun 4, 2014

WalterBright commented Jun 5, 2014

WalterBright commented Jul 27, 2014

Issue 10985 - Compiler doesn't attempt to inline non-templated functions from libraries (even having the full source) #2561

Issue 10985 - Compiler doesn't attempt to inline non-templated functions from libraries (even having the full source) #2561

Conversation

9rnsr commented Sep 16, 2013

ibuclaw commented Sep 16, 2013

9rnsr commented Sep 16, 2013

ibuclaw commented Sep 16, 2013

MartinNowak commented Sep 16, 2013

ibuclaw commented Sep 16, 2013

ibuclaw commented Sep 16, 2013

ghost commented Sep 16, 2013

9rnsr commented Sep 17, 2013

9rnsr commented Sep 17, 2013

ibuclaw commented Sep 18, 2013

9rnsr Sep 19, 2013

Choose a reason for hiding this comment

MartinNowak commented Sep 19, 2013

rainers commented Sep 21, 2013

MartinNowak commented Oct 15, 2013

9rnsr commented Oct 15, 2013

MartinNowak commented Oct 15, 2013

DmitryOlshansky commented Nov 28, 2013

MartinNowak commented Nov 28, 2013

Safety0ff commented Mar 18, 2014

ghost commented Mar 18, 2014

9rnsr commented May 16, 2014

MartinNowak May 16, 2014

Choose a reason for hiding this comment

9rnsr May 17, 2014

Choose a reason for hiding this comment

MartinNowak May 17, 2014

Choose a reason for hiding this comment

9rnsr May 17, 2014

Choose a reason for hiding this comment

9rnsr May 17, 2014

Choose a reason for hiding this comment

MartinNowak commented May 16, 2014

9rnsr commented May 17, 2014

MartinNowak commented May 17, 2014

WalterBright commented Jun 4, 2014

WalterBright commented Jun 5, 2014

WalterBright commented Jul 27, 2014