SIMD backend: implement signMask

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dougc, Assigned: dougc)

Tracking

unspecified
mozilla34
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

5 years ago
Implement the SIMD signMask operation.
Assignee

Comment 1

5 years ago
The same instruction appears appropriate for float32x4 and int32x4. No consideration has been given to float64x2 yet.
Attachment #8480539 - Flags: review?(sunfish)
Comment on attachment 8480539 [details] [diff] [review]
SIMD backend: implement signMask

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

Thank you for doing this! A few remarks that will hopefully help sunfish's review :)

::: js/src/jit/Lowering.cpp
@@ +3732,5 @@
> +{
> +    MOZ_ASSERT(IsSimdType(ins->input()->type()));
> +    MOZ_ASSERT(ins->type() == MIRType_Int32);
> +
> +    LUse use = useAtStart(ins->input());

see remark in CodeGen

::: js/src/jit/MIR.h
@@ +1365,5 @@
> +    MSimdSignMask(MDefinition *obj)
> +      : MUnaryInstruction(obj)
> +    {
> +        JS_ASSERT(IsSimdType(obj->type()));
> +        setResultType(MIRType_Int32);

can it be movable, please?

::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
@@ +2192,5 @@
>      return true;
>  }
>  
>  bool
> +CodeGeneratorX86Shared::visitSimdSignMask(LSimdSignMask *ins)

can you add a mention in the name that it's for x4 types? float64x2 will use movmskpd.

@@ +2195,5 @@
>  bool
> +CodeGeneratorX86Shared::visitSimdSignMask(LSimdSignMask *ins)
> +{
> +    FloatRegister input = ToFloatRegister(ins->input());
> +    Register output = ToRegister(ins->output());

not sure about that, but shouldn't it be useRegisterAtStart rather than useAtStart if you convert it into a register here?

@@ +2197,5 @@
> +{
> +    FloatRegister input = ToFloatRegister(ins->input());
> +    Register output = ToRegister(ins->output());
> +
> +    masm.movmskps(input, output);

there might be missing parts of your patch, as i can't see any changes to MacroAssembler-x86-shared and the levels below
Assignee: nobody → dtc-moz
Status: NEW → ASSIGNED
Assignee

Comment 3

5 years ago
Address feedback, thank you.
Attachment #8480539 - Attachment is obsolete: true
Attachment #8480539 - Flags: review?(sunfish)
Attachment #8480566 - Flags: review?(sunfish)
Comment on attachment 8480566 [details] [diff] [review]
SIMD backend: implement signMask

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

Cool.

::: js/src/jit/MIR.h
@@ +1361,5 @@
> +// Extracts the sign bits from a given vector, returning an MIRType_Int32.
> +class MSimdSignMask : public MUnaryInstruction
> +{
> +  protected:
> +    MSimdSignMask(MDefinition *obj)

This constructor should have an explicit keyword.

@@ +1364,5 @@
> +  protected:
> +    MSimdSignMask(MDefinition *obj)
> +      : MUnaryInstruction(obj)
> +    {
> +        JS_ASSERT(IsSimdType(obj->type()));

Please use MOZ_ASSERT instead of JS_ASSERT for new code.
Attachment #8480566 - Flags: review?(sunfish) → review+
Assignee

Comment 5

5 years ago
Address reviewer feedback. Thank you for the quick review. Carrying forward r+.
Attachment #8480566 - Attachment is obsolete: true
Attachment #8480865 - Flags: review+
Assignee

Comment 6

5 years ago
Rebase. Carrying forward r+.
Attachment #8480865 - Attachment is obsolete: true
Attachment #8481199 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6a78c4812f10
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.