520714 - nanojit: distinguish 64-bit int and float loads/stores

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Currently LIR_ldq/LIR_stqi are used for both 64-bit int and 64-bit float load/stores.  This is inconsistent with the rest of LIR, and leads to problems like bug 520208/bug 517939.

We should split them.  Under the naming scheme suggested in bug 504506 they would become LIR_ldq/LIR_ldf and LIR_stq/LIR_stf.

As part of this bug, we should also better differentiate between LIR_quad and LIR_float.  Eg. don't feed both of them into asm_quad().  (In particular, the X64 backend doesn't distinguish the two cases.)

Comment 1

[Nicholas Nethercote [:njn] (On Long Service Leave)]

I hit another bug caused by this today.  Imagine this sequence of code:

  a = ldqc m
  ...
  b = ldqc m
  ...
  c = neg a
  ...
  d = fneg b

After CSE runs, we have this:

  a = ldqc m
  ...
  c = neg a
  ...
  d = fneg a

Working backwards, as the assembler does:  'a' gets assigned to an FpReg because it's used in the fneg.  Then we need to assign it to a GpReg because it's used in the neg.  This causes an assertion failure in NativeX64.cpp.

If we had LIR_ldqc and LIR_ldfc we wouldn't have erroneously CSE'd them and the problem would be avoided.  We may not have hit this in the wild because the CSEable loads are rarely used.  I hit it with lirasm --random on a 1,000,000 instruction block.

Fuzzing, FTW!  Also, boo to lax type systems!

Comment 3

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: NJ patch

This patch adds ldf/ldfc/stfi to Nanojit.  I have done it so that the
generated code isn't actually changed -- the Q and F variants are still
treated the same.  I plan to do changes that affect codegen in a follow-up
bug, as this should make any breakage easier to identify.

I'll post a TM patch shortly.

I don't have a TR patch.  I tried to do one but was stymied by the mop_*
stuff which I don't understand at all.  Would one of the Adobe guys mind
concocting a TR patch?  It shouldn't be too hard for someone who understands
the mop_* stuff.  Thanks.

[Also, FWIW the code in comment 1 is type-incorrect -- since 'a' and 'b' are
really the same value they should be used in such a way that they have the
same type.  lirasm --random shouldn't have generated type-incorrect code 
like that (and this patch fixes it accordingly).  LIR type-checking would 
also have caught that one.]

Comment 4

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: TM patch

Comment 5

[Steven Johnson]

(In reply to comment #1)
> Also, boo to lax type systems!

Yeah... imagine, if only JavaScript had some sort of optional typing system... :-)

Comment 6

[Steven Johnson]

(In reply to comment #3)
> I don't have a TR patch.  I tried to do one but was stymied by the mop_*
> stuff which I don't understand at all.  Would one of the Adobe guys mind
> concocting a TR patch?  It shouldn't be too hard for someone who understands
> the mop_* stuff.  Thanks.

I'll do it.

Comment 7

[Nicholas Nethercote [:njn] (On Long Service Leave)]

(In reply to comment #5)
> (In reply to comment #1)
> > Also, boo to lax type systems!
> 
> Yeah... imagine, if only JavaScript had some sort of optional typing system...
> :-)

It's possible for LIR to have a strong type system, that's totally independent of JS.  This bug is one step towards that!


(In reply to comment #6)

> I'll do it.

Thanks!

Comment 8

[Steven Johnson]

Why is the store-float-64 opcode "LIR_stfi"? I always assumed the "i" was for "integer"...

Comment 9

[Steven Johnson]

Trying out a patch in TR, but I get kajillions asserts in the 386 backend: asm_load64/asm_store64 now assert that they shouldn't get the "q" variant (only the "f" variant), but Assembler still happily will feed those ops to them...

Comment 10

[Steven Johnson]

Attachment: TR patch

I think this is all that's needed... but TR won't run at all due to the previously-mentioned fits of assertions. Either I integrated something very incorrectly, or the previous patch is not fully baked.

Comment 11

[Steven Johnson]

Comment on attachment 418747 [details] [diff] [review]
NJ patch

for 

    if (value->isop(LIR_qjoin) && op == LIR_stfi)

why are we ignoring LIR_stqi here? Seems like it could handle this too.

Comment 12

[Steven Johnson]

Attachment: TR patch, rev 2

ok, I'm wrong, new patch that actually works.

One upshot of this change is that ldq,ldqc,stqi,quad are now valid only if NANOJIT_64BIT is defined... IMHO we should add asserts to various points in LirWriter (insLoad,insStore,insImmq) to assert faster and make this restriction clearer. (In fact, insImmq only really needs to exist for NANOJIT_64BIT builds, but that probably would make for more-awkward-than-necessary code.)

Comment 13

[Edwin Smith]

Comment on attachment 418765 [details] [diff] [review]
TR patch, rev 2

R+ contingent on one nit:

CodegenLIR.cpp:1191  The asserts to not use LIR_quad on 32bit systems should go in ValidateWriter.   Same comment elsewhere for LIR_stqi, ldqc, etc.  If you want extra checking, also add asserts to ValidateReader.

Comment 14

[Rick Reitmaier]

Comment on attachment 418747 [details] [diff] [review]
NJ patch

r+ but there is sufficient change that it would be good to flush it through the sandbox before landing.

http://tamarin-builds.mozilla.org/build_trigger/requestbuild.cfm

Let me know if you need a hand.

Comment 15

[Andreas Gal :gal]

Comment on attachment 418747 [details] [diff] [review]
NJ patch


>     {
>-        LOpcode op = value->isQuad() ? LIR_stqi : LIR_sti;
>+        // Determine which kind of store should be used for 'value' based on
>+        // its type.
>+        LOpcode op = LOpcode(0);
>+        switch (retTypes[value->opcode()]) {
>+        case LTy_I32:   op = LIR_sti;   break;
>+        case LTy_I64:   op = LIR_stqi;  break;
>+        case LTy_F64:   op = LIR_stfi;  break;
>+        case LTy_Void:  NanoAssert(0);  break; 
>+        default:        NanoAssert(0);  break;
>+        }

Use a table maybe for denser code and fewer branch misses? Or define LTy_I32 as LIR_sti and so on and then just cast?

Sorry for the delay. Looks great.

Comment 16

[Nicholas Nethercote [:njn] (On Long Service Leave)]

I ran it through the Tamarin try server, all green except for one timeout at the end of the ReleaseDebugger run
(see http://tamarin-builds.mozilla.org/tamarin-redux/builders/mac-ppc-10.5b-test-sandbox/builds/43/steps/Testsuite_ReleaseDebugger/logs/stdio) which I hope is not my fault.

> Use a table maybe for denser code and fewer branch misses? Or define LTy_I32 as
> LIR_sti and so on and then just cast?

Hmm, it's a small switch and not executed that often;  I think that would be premature optimization.

Pushed to NJ-central:

http://hg.mozilla.org/projects/nanojit-central/rev/a19809f7ba60

Comment 17

[Nicholas Nethercote [:njn] (On Long Service Leave)]

http://hg.mozilla.org/tracemonkey/rev/2ed8973352a8

Comment 18

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Previous TM landing was the NJ-specific patch.  This is the TM-specific patch:

http://hg.mozilla.org/tracemonkey/rev/6d00d56ab3ed

Comment 19

[Steven Johnson]

TR: http://hg.mozilla.org/tamarin-redux/rev/441d755b0210

Comment 20

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/2ed8973352a8