Closed Bug 513838 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: dvander, Unassigned)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

Attached patch proposed changesSplinter Review
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.
Attachment #397778 - Flags: review?(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?
Attachment #397778 - Flags: review?(gal)
Attachment #397778 - Flags: review?(edwsmith)
Attachment #397778 - Flags: review+
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.
Attachment #397778 - Flags: review?(rreitmai)
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.
Attachment #397778 - Flags: review?(rreitmai) → review+
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.
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).
Attachment #397778 - Flags: review?(edwsmith) → review+
Blocks: 515822
http://hg.mozilla.org/mozilla-central/rev/95e022767005
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: