615871 - (float&float4_JIT) Add JIT support for the float & float4 type

Status: RESOLVED FIXED

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), enhancement)

Description

[Virgil Palanciuc]

User-Agent:       Jakarta Commons-HttpClient/3.0 (Deskzilla Lite/2.1.5508.129)
Build Identifier: 

In order to have effictient support of float, we need to update the JIT - add 
new LIR instructions, handle new opcodes/new semnatics of existing opcodes, etc.

Reproducible: Always

Comment 1

[Virgil Palanciuc]

Attachment: NanoJIT changes for float - new LIR opcodes

need to apply patch from bug 616169 first

Comment 2

[Virgil Palanciuc]

Attachment: NanoJIT changes for float - generic assembler changes

Comment 3

[Virgil Palanciuc]

Attachment: NanoJIT changes for float - i386-specific

Comment 4

[Virgil Palanciuc]

Attachment: NanoJIT changes for float - x64-specific

Comment 5

[Virgil Palanciuc]

Attachment: NanoJIT changes for float - PPC-specific

Only stub methods done; the PPC port is non-function, this is just so that it links & xcode may produce universal binaries (so that our QA workflows/build system remains unchanged)

Comment 6

[Virgil Palanciuc]

Attachment: NanoJIT changes for float - PPC-specific

resubmitted, as "patch"

Comment 7

[Virgil Palanciuc]

Attachment: lirasm changes for float support

Comment 8

[Virgil Palanciuc]

Attachment: lirasm tests for new float-related opcodes

Comment 9

[Virgil Palanciuc]

Attachment: NanoJIT changes for float - other platforms

This is just a stub so that other platforms would compile & run (as long as no float code is involved). It doesn't really add float support.

Comment 10

[Virgil Palanciuc]

Comment on attachment 532983 [details] [diff] [review]
NanoJIT changes for float - PPC-specific

obsoleted by the "other platforms" patch

Comment 11

[Virgil Palanciuc]

Attachment: NanoJIT changes for float - new LIR opcodes

Comment 12

[Virgil Palanciuc]

Attachment: NanoJIT changes for float - i386-specific

Comment 13

[Virgil Palanciuc]

Attachment: NanoJIT changes for float - generic assembler changes

Comment 14

[Virgil Palanciuc]

Attachment: NanoJIT changes for float - x64-specific

Comment 15

[Virgil Palanciuc]

Attachment: NanoJIT changes for float - other platforms

Comment 16

[Virgil Palanciuc]

Attachment: NanoJIT changes for float - i386-specific

changed mostly because of rebasing

Comment 17

[Virgil Palanciuc]

FWIW - my MQ "series" file (i.e. the order of the patches) is:
bug-616169.LIRopcodeTBL.diff      // from bug 616169, (r+) but not integrated
bug-616169.lirasm.diff #+nanojit  // from bug 616169
bug-615871.floatNanoJIT.diff   // NanoJIT changes for float - new LIR opcodes
bug-615871.nJIT_Assembler.diff  // NanoJIT changes for float - generic assembler 
bug-615871.nJIT_x64.diff     // NanoJIT changes for float - x64
bug-615871.nJIT_i386.diff    // NanoJIT changes for float - x64
bug-615871.nJIT_others.diff  // NanoJIT changes for float - ARM, PPC etc. - just stubs
bug-615871.nJIT_lirasm.diff // lirasm changes for float support 
bug-615871.nJIT_tests.diff // lirasm tests for new float-related opcodes

Comment 18

[Virgil Palanciuc]

Comment on attachment 533910 [details] [diff] [review]
lirasm changes for float support

cancel review request, will update soon to float+float4

Comment 19

[Virgil Palanciuc]

Attachment: NanoJIT changes for float - new LIR opcodes

Comment 20

[Virgil Palanciuc]

Attachment: NanoJIT changes for float/float4 - generic assembler changes

Comment 21

[William Maddox]

Comment on attachment 569026 [details] [diff] [review]
NanoJIT changes for float - new LIR opcodes

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

Looks good, except for the use of f4_eq_i() in CseFilter::findImmF4() where a bitwise comparison should be used.  It should not be used in findImmF4() for the same reason that you note in CseFilter::insImmF4(), i.e., NaNs and -0 vs. +0.  There are also a few places in comments where  you call Float4 a "128-bit float" -- it should always be described as a vector.

The following usage is questionable:
    const uint32_t* up = reinterpret_cast<const uint32_t*>(&a);

The compiler may assume that *up does not alias with an object declared as a type other than uint32_t.  It would be preferable to coerce via a union, as done elsewhere.

align_float_comparisons -> align_eqf  (for consistency)
essentialy -> essentially

Comment 22

[William Maddox]

Comment on attachment 569027 [details] [diff] [review]
NanoJIT changes for float/float4 - generic assembler changes

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

This looks reasonable, but see comments and questions below prior to submitting for review.  F+

The aligned allocator can be streamlined by noting that a pointer may be aligned like so:
    current_top = (current_top + align - 1) & ~(align - 1)

We are already rounding up allocations to 8-byte alignment.  Since this is a bump-pointer allocator, I presume we are doing it for the alignment, not to reduce fragmentation.  We should not add any padding here to the request size before aligning to the user-specified boundary.  In fact, I don't see any point for the extra 8-byte alignment in this case -- we should just pad to the alignment specified by the caller.  We should assert that the alignment >= 8, as it looks like you use only the size of the key to determine the alignment, which might conceivably in some future scenario be smaller than a pointer.

Use consistent whitespace, e.g., space after commas, no space after open paren or before close paren.  E.g., HashMap::keysEqual(). Don't include random whitespace cleanup in patches to be reviewed, however.  Commit cleanups separately, and keep the patches you send out for review clean.

Is asm_condd() meant to handle floats as well as doubles?

While Tamarin may not generate LIR_livef or LIR_lifef4, this will need to change when float/float4 support goes into Halfmoon.

Why is there support for an IMMD pool and an an IMMF4 pool, but no IMMF pool?

Comment 23

[Virgil Palanciuc]

f4_eq_i - you're right, good catch.
const uint32_t* vs. unions: I disagree, there are GCC bugs that make it crash when using unions, and besides Tamarin isn't ready for strict pointer aliasing anyway - there have been extensive discussions about this and AFIACT the decision is "we don't care about strict aliasing rules".

Ref aligning: I understand your point; I attempted "minimum modifications" so I kept nBytes aligned to 8 originally. Note that this guarantees that everything is at least 8-byte aligned, even if input alignment is less than 8. But yes, I think it's safe to consolidate alignment functions into a single one.

asm_condd: yes. Naming is a bit unfortunate, I know - but the difference between float&double did not justify a new function (unlike float4 which is quite different).

IMMF pool: I punted, I reuse the IMMD pool in the few cases where I need to. Thing is, float constants often fit in instruction immediates, but with float4 it's a completely different issue, no platform will allow you to specify a 128-bit immediate.

Comment 24

[Virgil Palanciuc]

"allocation functions"  (with & without explicit alignment), not "alignment functions"

Comment 25

[Virgil Palanciuc]

Ok, I started rewriting the allocator functions, but then I gave up: there are assertions that the allocated quantities are multiple of 8, I don't want to break assumptions made somewhere else, and this doesn't seem important enough.

Comment 26

[Lars T Hansen]

(In reply to William Maddox from comment #21)

> The following usage is questionable:
>     const uint32_t* up = reinterpret_cast<const uint32_t*>(&a);
> 
> The compiler may assume that *up does not alias with an object declared as a
> type other than uint32_t.  It would be preferable to coerce via a union, as
> done elsewhere.

FWIW: There are three things about the state of things that make me make the opposite recommendation (ie leave it alone):

- The union trick is known to cause GCC 4.2 to crash when compiling FR on Mac,
  and this is still the primary compiler for us on that platform.  Ergo, 
  more likely than not such a change may be reverted.
- I am not convinced that the union trick is actually sufficient to allow
  strict aliasing to be enabled, the way it is frequently used throughout the 
  code, even if it were employed generally.
- AVM+ and FR do not currently work when compiled with strict aliasing enabled,
  so making changes that benefit strict aliasing is premature optimization.

Comment 27

[Tamarin Bot]

changeset: 6801:66466286527d
user:      Virgil Palanciuc <[e-mail]>
summary:   imported patch bug-615871.nJIT_Assembler.diff

http://hg.mozilla.org/tamarin-redux/rev/66466286527d

Comment 28

[Tamarin Bot]

changeset: 6802:58cdb54f2569
user:      Virgil Palanciuc <[e-mail]>
summary:   imported patch bug-615871.nJIT_others.diff

http://hg.mozilla.org/tamarin-redux/rev/58cdb54f2569

Comment 29

[Tamarin Bot]

changeset: 6803:b3de6aae82ac
user:      Virgil Palanciuc <[e-mail]>
summary:   imported patch bug-615871.nJIT_i386.diff

http://hg.mozilla.org/tamarin-redux/rev/b3de6aae82ac

Comment 30

[Tamarin Bot]

changeset: 6804:8eec33b1b726
user:      Virgil Palanciuc <[e-mail]>
summary:   imported patch bug-615871.nJIT_x64.diff

http://hg.mozilla.org/tamarin-redux/rev/8eec33b1b726

Comment 31

[Tamarin Bot]

changeset: 6805:63d0b1dccccf
user:      Virgil Palanciuc <[e-mail]>
summary:   imported patch bug-615871.nJIT_ARM.diff

http://hg.mozilla.org/tamarin-redux/rev/63d0b1dccccf

Comment 32

[Nicholas Nethercote [inactive]]

Pretty sure this can be closed now.