Closed Bug 875720 Opened 11 years ago Closed 11 years ago

Sunspider 1.0 math-cordic regression from bug 875276

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox24 + fixed

People

(Reporter: jandem, Assigned: djvj)

References

Details

Attachments

(2 files)

Attached file Stand-alone testcase
Bug 875276 regressed SS 1.0 math-cordic, it's now about 10x slower (2.5 ms -> 25 ms) with parallel compilation enabled. Without parallel compilation it's worse (2 ms -> 78 ms).

It looks like we keep recompiling the same script. I'm attaching a slightly modified, stand-alone version of the benchmark.

AWFY did not catch it because it apparently runs SS 0.9.1.
Flags: needinfo?(bhackett1024)
We have:

function FLOAT(X)
{
    return X / 65536.0;
}

Then we call FLOAT(Y), where Y is always an int32 (30765). The DIV is lowered as LDivPowTwoI, but that's wrong because it will always bailout (remainder != 0).
So the problem is that Baseline adds an IC stub for the JSOP_DIV in FLOAT(X) that handles both integers and doubles. This means that if at some point integer operands show up, the stub handles them and we don't call MonitorOverflow...

Ion should probably inspect the baseline cache and specialize as double. What do you think?
(In reply to Jan de Mooij [:jandem] from comment #2)
> Ion should probably inspect the baseline cache and specialize as double.
> What do you think?

This sounds good.  We currently use TypeMonitorOverflow to indicate this to Ion, which will only work if the call that attaches the stub had int32 inputs.  It looks like FLOAT() will be called with both int32s and doubles, and the stub is attached when the input is a double so no overflow monitor occurs.  I don't think that TypeMonitorOverflow adds anything in the presence of the baseline stubs and we should work towards weaning off the former entirely.
Flags: needinfo?(bhackett1024)
If no-one else is taking this, I can whip up a quick fix.
Assignee: general → kvijayan
Attached patch Fix.Splinter Review
Fix is pretty self-explanatory.
Attachment #761009 - Flags: review?(jdemooij)
Comment on attachment 761009 [details] [diff] [review]
Fix.

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

Cool, thanks.
Attachment #761009 - Flags: review?(jdemooij) → review+
Could it be this introduced a 2% regression on awfy x86? It's really noisy so I can't really say which benchmarks, but the score was 17000 before and now 16700.
https://hg.mozilla.org/mozilla-central/rev/6e11b247330e
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
(In reply to Hannes Verschore [:h4writer] from comment #8)
> Could it be this introduced a 2% regression on awfy x86? It's really noisy
> so I can't really say which benchmarks, but the score was 17000 before and
> now 16700.

Nice find!  Not directly related to this bug, but there's definitely a bug in baseline that's causing this.
Depends on: 882925
You need to log in before you can comment on or make changes to this bug.