Closed Bug 520714 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: njn, Assigned: njn)

References

Details

(Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin)

Attachments

(3 files, 1 obsolete file)

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.)
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!
Duplicate of this bug: 412615
Blocks: 412615
Depends on: 527085
Blocks: 516855
No longer blocks: 516855
Blocks: 534309
Attached patch NJ patchSplinter Review
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.]
Attachment #418747 - Flags: review?(gal)
Attached patch TM patchSplinter Review
Attachment #418748 - Flags: review?(gal)
Attachment #418747 - Flags: review?(rreitmai)
Blocks: 536291
(In reply to comment #1)
> Also, boo to lax type systems!

Yeah... imagine, if only JavaScript had some sort of optional typing system... :-)
(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.
(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!
Why is the store-float-64 opcode "LIR_stfi"? I always assumed the "i" was for "integer"...
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...
Attached patch TR patch (obsolete) — Splinter Review
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.
Attachment #418755 - Flags: review?(nnethercote)
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.
Attached patch TR patch, rev 2Splinter Review
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.)
Attachment #418765 - Flags: review?(edwsmith)
Attachment #418755 - Attachment is obsolete: true
Attachment #418755 - Flags: review?(nnethercote)
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.
Attachment #418765 - Flags: review?(edwsmith) → review+
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.
Attachment #418747 - Flags: review?(rreitmai) → review+
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.
Attachment #418747 - Flags: review?(gal) → review+
Attachment #418748 - Flags: review?(gal) → review+
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
Whiteboard: fixed-in-nanojit
http://hg.mozilla.org/tracemonkey/rev/2ed8973352a8
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey
Previous TM landing was the NJ-specific patch.  This is the TM-specific patch:

http://hg.mozilla.org/tracemonkey/rev/6d00d56ab3ed
Blocks: 463137
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin
http://hg.mozilla.org/mozilla-central/rev/2ed8973352a8
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.