If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

SIMD x86-x64 backend: implement from / fromBits and add them to Odin

RESOLVED FIXED in mozilla35

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bbouvier, Assigned: bbouvier)

Tracking

(Blocks: 1 bug)

Trunk
mozilla35
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Assignee)

Description

3 years ago
int32x4.fromFloat32x4(float32x4 x) / float32x4.fromInt32x4(x) create a new vector with values being *converted* (~static_cast).

int32x4.fromFloat32x4Bits(float32x4 x) / float32x4.fromInt32x4Bits(x) create a new vector with bits being *reinterpreted* (~reinterpret_cast)
(Assignee)

Updated

3 years ago
Blocks: 1068028
(Assignee)

Comment 1

3 years ago
Created attachment 8493743 [details] [diff] [review]
1) SimdConvertFrom

- I wasn't really inspired for the naming. Maybe simply SimdConvert would be enough? The From suffix refers to the name in the spec / polyfill, but that could actually be removed.
- the only issue with the spec is with int32x4.fromFloat32x4(x). This function is supposed to map to cvtps2dq or cvttps2dq (i've chosen the latter for consistency with convertFloat32ToInt32()), which return the undefined int32 value (0x80000000) for float values that are bigger than an int32. There is a big comment and a link to bug 1068028 in the code, so that should be enough to catch up on the spec later.
Attachment #8493743 - Flags: review?(sunfish)
(Assignee)

Comment 2

3 years ago
Created attachment 8493744 [details] [diff] [review]
2) SimdConvertFrom, Support in Odin
Attachment #8493744 - Flags: review?(luke)
(Assignee)

Comment 3

3 years ago
Created attachment 8493746 [details] [diff] [review]
3) SimdCast

This one doesn't even generate code, how nice is that :)

Simply using redefine() seems to be enough from my testing in Odin (even with a use of the cast operand afterwards), but will it always be safe in all contexts?
Attachment #8493746 - Flags: review?(sunfish)
(Assignee)

Comment 4

3 years ago
Created attachment 8493749 [details] [diff] [review]
4) SimdCast: support in Odin
Attachment #8493749 - Flags: review?(luke)
Comment on attachment 8493743 [details] [diff] [review]
1) SimdConvertFrom

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

::: js/src/jit/MIR.h
@@ +1361,5 @@
>      }
>  };
>  
> +// Converts all lanes of a given vector into the type of another vector
> +class MSimdConvertFrom : public MUnaryInstruction

How about just MSimdConvert?

@@ +1385,5 @@
> +    }
> +    bool congruentTo(const MDefinition *ins) const {
> +        return congruentIfOperandsEqual(ins);
> +    }
> +};

I haven't been consistently asking for it, but we could start putting ALLOW_CLONE declarations in operators like this. ALLOW_CLONE enables loop unrolling, and maybe eventually jump threading and loop rotation.

::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
@@ +2185,5 @@
> +    FloatRegister in = ToFloatRegister(ins->input());
> +    FloatRegister out = ToFloatRegister(ins->output());
> +    masm.convertFloat32x4ToInt32x4(in, out);
> +    return true;
> +}

It seems likely that these operations will be implementable in the macro-assembler for any sane target, so can we move these two CodeGenerator functions into platform-independent CodeGenerator.cpp?

::: js/src/jit/shared/MacroAssembler-x86-shared.h
@@ +468,5 @@
>      }
>  
> +    void convertFloat32x4ToInt32x4(FloatRegister src, FloatRegister dest) {
> +        // TODO: Note that if the conversion failed (because the converted
> +        // result is larger than the maximum signed int32), this will return

... or less than the least signed int32, or NaN, for completeness.
Attachment #8493743 - Flags: review?(sunfish) → review+
Comment on attachment 8493746 [details] [diff] [review]
3) SimdCast

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

::: js/src/jit/Lowering.cpp
@@ +3740,5 @@
>  bool
> +LIRGenerator::visitSimdCast(MSimdCast *ins)
> +{
> +    MOZ_ASSERT(IsSimdType(ins->type()) && IsSimdType(ins->input()->type()));
> +    return redefine(ins, ins->input());

The key requirement for redefine is that the input and output are in the same register class. On all targets we can reasonably anticipate today, this is true, so we should be ok here.

::: js/src/jit/MIR.h
@@ +1388,5 @@
>      }
>  };
>  
> +// Casts bits of a vector input to another SIMD type (doesn't generate code).
> +class MSimdCast : public MUnaryInstruction

Let's rename this to MSimdReinterpretCast to avoid ambiguity.

@@ +1412,5 @@
> +    }
> +    bool congruentTo(const MDefinition *ins) const {
> +        return congruentIfOperandsEqual(ins);
> +    }
> +};

This class can also get an ALLOW_CLONE.
Attachment #8493746 - Flags: review?(sunfish) → review+

Updated

3 years ago
Attachment #8493744 - Flags: review?(luke) → review+

Comment 7

3 years ago
Comment on attachment 8493749 [details] [diff] [review]
4) SimdCast: support in Odin

Perfect
Attachment #8493749 - Flags: review?(luke) → review+
(Assignee)

Comment 8

3 years ago
Addressed all nits, except for moving the codegen method to the shared codegen: a lot of other methods are in the same situation, so I'd really prefer have the ARM macroassembler methods implemented before moving them to shared codegen.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1926709eaf90
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fd0ef779e8a1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c965698b314e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e2c803c2aeec
https://hg.mozilla.org/mozilla-central/rev/1926709eaf90
https://hg.mozilla.org/mozilla-central/rev/fd0ef779e8a1
https://hg.mozilla.org/mozilla-central/rev/c965698b314e
https://hg.mozilla.org/mozilla-central/rev/e2c803c2aeec
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.