859257 - Clang miscompiles MConstant::computeRange / ToInt64, hurts performance

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Below is a testcase reduced from a Marsaglia RNG written in JS. I get these numbers:

Clang 3.2 debug:  144 ms
Clang 3.2 opt  : 2874 ms

So the opt build is a lot slower. Nightly builds seem to have the same problem. 

It looks like Clang miscompiles this line in MConstant::computeRange():

    int64_t integral = ToInt64(d);

Integral ends up being 0 instead of 36969 and this messes up range analysis. I think in ToIntWidth, we end up reading du.s.lo here instead of du.s.hi:

    di_h = du.s.hi;

I tried to move the ToInt64 call into a JS_NEVER_INLINE function, but that didn't help.

function marsaglia(m_z) {
    for (var i = 0; i < 100000000; ++i) {
        m_z = (36969 * m_z)|0;
    }
    return m_z;
}
var t = new Date;
var res = marsaglia(2);
print(new Date - t);
print(res);

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

Marking DoublePun values as |volatile| seems to avoid the problem..

Comment 2

[Boris Zbarsky [:bzbarsky]]

Nathan, do we need to upstream this to clang, or are we violating aliasing rules on our end somehow?  I was told to ping you now that respindola is no longer around....

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Boris Zbarsky (:bz) from comment #2)
> Nathan, do we need to upstream this to clang, or are we violating aliasing
> rules on our end somehow?  I was told to ping you now that respindola is no
> longer around....

I don't see any aliasing violations here; there's no pointer cast games, which usually is the source of aliasing woes.

Jan, what platform are you seeing the miscompilation on?

Comment 4

[Nathan Froyd [:froydnj]]

Also, Jan, if you could provide preprocessed source for RangeAnalysis.cpp, that would be helpful.  Something like:

  make -C js/src RangeAnalysis.i

should do the trick.

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Nathan Froyd (:froydnj) from comment #3)
> Jan, what platform are you seeing the miscompilation on?

OS X, 32-bit build, Clang 3.2:

$ clang++ --version
clang version 3.2 (tags/RELEASE_32/final)
Target: x86_64-apple-darwin11.4.2
Thread model: posix

Nightly builds seem to have the same problem though.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: RangeAnalysis.i.bz2

(In reply to Nathan Froyd (:froydnj) from comment #4)
> Also, Jan, if you could provide preprocessed source for RangeAnalysis.cpp,
> that would be helpful.

Attaching RangeAnalysis.i.bz2

Comment 7

[Nathan Froyd [:froydnj]]

Thanks Jan.  Can you tell me what the double that gets misconverted is?

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Nathan Froyd (:froydnj) from comment #7)
> Thanks Jan.  Can you tell me what the double that gets misconverted is?

It's 36969.0 (int32 stored as double), note that this value is from the JS testcase in comment 0.

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 734552 [details] [diff] [review]
Patch

Review of attachment 734552 [details] [diff] [review]:
-----------------------------------------------------------------

Doesn't make me happy, but if it works, maybe it's enough for now.  I'd like to see this random nuke-from-orbit volatility gone really quick, tho -- I'd be surprised if this doesn't hurt perf on To* operations in general, even if it makes them correct.

Comment 10

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c0d13ce4c4a

Comment 11

[Nathan Froyd [:froydnj]]

Attachment: include jscpucfg.h in NumericConversions.h so DoublePun definitions are correct

I've been debugging this and I don't see anything clang is doing that is
obviously wrong...but I did notice this in the preprocessed output:

union DoublePun {
    struct {



        uint32_t hi, lo;

    } s;
    uint64_t u64;
    double d;
};

which comes from:union DoublePun {
    struct {
        uint32_t lo, hi;
        uint32_t hi, lo;
    } s;
    uint64_t u64;
    double d;
};

...and that looks like we're not picking up the right definitions from jscpucfg.h.

The breakage here is strictly limited to RangeAnalysis.cpp, since it
includes vm/NumericConversions.h without including other js headers
prior to it.  On the plus (minus?) side, this means that it wasn't just
a clang issue: all platforms should benefit from this fix.

Testing with the fix on OS X shows the opt config to be slightly faster
than debug:

bld-lion-r5-054:~ cltbld$ ./build-js-debug/js test.js
176
869830658
bld-lion-r5-054:~ cltbld$ ./build-js-opt/js test.js
173
869830658

If you'd rather have a separate backout of Jan's patch followed by the

There are other issues here (FPU_IS_ARM_FPA seems to be undefined by
*anything*, including configury; we should be getting DoublePun
definitions from One True Source, etc. etc.), but those can be handled
in separate bugs.  I'll file a followup for ditching FPU_IS_ARM_FPA;
there's already open bugs on enhancing FloatingPoint.h and consolidating
the DoublePun definitions.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c0d13ce4c4a

Comment 13

[Jan de Mooij [:jandem]]

(In reply to Nathan Froyd (:froydnj) from comment #11)
> ...and that looks like we're not picking up the right definitions from
> jscpucfg.h.

Very nice find, thanks a lot. I do wonder though why adding "volatile" avoids the problem, any idea?

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to Jan de Mooij [:jandem] from comment #13)
> (In reply to Nathan Froyd (:froydnj) from comment #11)
> > ...and that looks like we're not picking up the right definitions from
> > jscpucfg.h.
> 
> Very nice find, thanks a lot. I do wonder though why adding "volatile"
> avoids the problem, any idea?

That's a good question.  I'm not really sure.  "volatile" shouldn't be changing anything, since we have the wrong structure definition in the first place!

My guess is that swapping the structure members around somehow enabled the compiler to do some aggressive-but-not-valid transformations.  And that the results of those optimizations made RangeAnalysis exhibit particularly bad behavior.  "volatile" probably prevented propagation of the facts the compiler needed to do those transformations.  Which means that RangeAnalysis exhibited normal (if wrong?) behavior once again.

It'd be interesting to see where the opt build--before any of the fixes from this bug--was spending its time running that testcase.  I think it's a good bet something in RangeAnalysis was going haywire.

Comment 15

[Jeff Walden [:Waldo]]

Comment on attachment 736918 [details] [diff] [review]
include jscpucfg.h in NumericConversions.h so DoublePun definitions are correct

Review of attachment 736918 [details] [diff] [review]:
-----------------------------------------------------------------

Sigh.  Good enough for now.

Comment 16

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab6e995676bd

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ab6e995676bd

Comment 18

[Nathan Froyd [:froydnj]]

Jan pointed out this morning that the fix I committed was quite different from the patch Waldo reviewed: my commit simply added the #include without removing the conditionals for __clang__.  I blame my poor rebasing skills.

I've fixed that up by backing out Jan's original commit:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f10f33fc2e93

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f10f33fc2e93