513838 - TM: Add LIR_float so we can type check 64-bit values in demotion path

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: proposed changes

The problem is that we sniff LIR for quad(0) in the demotion pipeline and try to use integers instead, but quad(0) could be either a 64-bit pointer or a 64-bit floating point 0. So if there's a 64-bit TT_NULL we truncate the pointer to 32-bit. We need to a new 64-bit type that means "only floating point numbers".

To minimize the impact of this patch, the new LIR_float is just like LIR_quad. All of the *q and *Quad operations can take a LIR_float. The only exception is the new isconstf() which only returns true for LIR_float.

Because insImmf() now inserts LIR_float, that is a small compatibility problem for NJ embedders that call insImmf() and expect precisely a LIR_quad. Other than that, checking the type is basically opt-in.

This passes x86 trace-tests and fixes a bunch of failures on x64.

Aside: In a future patch we could use LIR_float to assist register allocation in asm_quad. Also, I'd prefer to have LIR_quad and LIR_float be totally separate - so everything in the backend is much more type safe. But that's a lot of work for little short-term gain, and would be a much bigger divergence problem between us and tamarin-redux.

Comment 1

[Andreas Gal :gal]

Comment on attachment 397778 [details] [diff] [review]
proposed changes

I love it. I think this makes LIR a lot easier to analyze and we have a bunch of existing cases where we distinguish q from f (ret, fret). I remember Ed wasn't a big fan though when we discussed this a year ago. Ed? Rick?

Comment 2

[Andreas Gal :gal]

Comment on attachment 397778 [details] [diff] [review]
proposed changes

Either-or review request. Not both. I wish there was a way to express this in bugzilla.

Comment 3

[Rick Reitmaier]

Comment on attachment 397778 [details] [diff] [review]
proposed changes

I like it - Not sure why you mention Ed didn't.
As I imagine the *cleanness* of knowing the underlying encoding would out-weigh (at least in my mind) the benefits of having a single entity representing any 64b encoded value.

Comment 4

[Andreas Gal :gal]

I think back then Ed was against typing above the level of word sizes (quad vs int), but with all the new quad ops for 64-bit, he might be more inclined now.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/tracemonkey/rev/95e022767005

Comment 6

[Edwin Smith]

the main problem with this comes when you are manipulating doubles at the bit level and need control flow paths that treat a 64bit value as either float or int64 depending on the bit patterns.

to support that we would also need a cast instruction (that assembles into an FPU<->GPU copy) in LIR.

in TR, we don't do that kind of manipulation -- it was a TT thing. 

I'm all for clean code and analyzable LIR, we just have to be careful not to over-constrain LIR by forcing frontends to insert casts where the cpu doesn't really need them.  (or, be sure that the backend optimizes them away effectively).

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/95e022767005

Comment 8

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/207baecef0db