IonMonkey: Inline SIMD.int32x4.or and SIMD.int32x4.xor calls.

RESOLVED FIXED in Firefox 38

Status

()

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

People

(Reporter: Victor Carlquist, Assigned: Victor Carlquist)

Tracking

Trunk
mozilla38
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

3 years ago
We need to inline the SIMD.int32x4.or and SIMD.int32x4.xor calls.
(Assignee)

Updated

3 years ago
Blocks: 1112155
Depends on: 1127929
(Assignee)

Comment 1

3 years ago
Created attachment 8558781 [details] [diff] [review]
Patch

Hi, 

This patch inline the SIMD.Int32x4.or and SIMD.Int32x4.xor calls.

Thanks :)
Attachment #8558781 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8558781 [details] [diff] [review]
Patch

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

This looks good, but this sounds like this would become redundant.
Have a look at builtin/SIMD.h [1], and see if we can avoid future copy & paste errors by using Macros.

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/builtin/SIMD.h#142,154,167
Attachment #8558781 - Flags: review?(nicolas.b.pierron) → feedback+
(Assignee)

Comment 3

3 years ago
Created attachment 8560138 [details] [diff] [review]
Macro

Hi!
I wrote a MACRO to generate the conditions.
Attachment #8558781 - Attachment is obsolete: true
Attachment #8560138 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8560138 [details] [diff] [review]
Macro

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

Would it be possible to modify the builtin/SIMD.h macros, such that we can reuse them here?
(Assignee)

Comment 5

3 years ago
Created attachment 8560644 [details] [diff] [review]
Macro

I needed to change the enum in MSimdArith to simplify the MACRO.
Attachment #8560138 - Attachment is obsolete: true
Attachment #8560138 - Flags: review?(nicolas.b.pierron)
Attachment #8560644 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8560644 [details] [diff] [review]
Macro

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

This looks good to me, I will request Benjamin feedback as well, to make sure he is aware of this change.

::: js/src/jit/MCallOptimize.cpp
@@ +36,5 @@
> +    return inlineSimdInt32x4BinaryArith(callInfo, native, MSimdBinaryArith::OP);
> +
> +#define INLINE_INT32X4_SIMD_BITWISE(OP)                                                 \
> +  if (native == js::simd_int32x4_##OP)                                                  \
> +    return inlineSimdInt32x4BinaryBitwise(callInfo, native, MSimdBinaryBitwise::OP##_);

Move these macro next to the only location where they are used, and #undef them after using them.
As they are temporary macros, append an underscore at the end.
Attachment #8560644 - Flags: review?(nicolas.b.pierron) → feedback+
Attachment #8560644 - Flags: feedback?(benj)
Comment on attachment 8560644 [details] [diff] [review]
Macro

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

Cool, thanks for doing this!

::: js/src/jit/MIR.h
@@ +1964,5 @@
> +        div,
> +        min,
> +        max,
> +        minNum,
> +        maxNum

Any chance you could use the macro here as well?

@@ +1976,5 @@
> +          case div:    return "Div";
> +          case min:    return "Min";
> +          case max:    return "Max";
> +          case minNum: return "MinNum";
> +          case maxNum: return "MaxNum";

And here as well? (capitalization isn't a big matter)
Attachment #8560644 - Flags: feedback?(benj) → feedback+
(Assignee)

Comment 8

3 years ago
Created attachment 8561544 [details] [diff] [review]
Macro

Hi, I made the changes that you and Benjamin suggested :)
Attachment #8560644 - Attachment is obsolete: true
Attachment #8561544 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8561544 [details] [diff] [review]
Macro

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

This patch looks good :)

Fix the comments, attach the newer version here, and push the patch to the try server. (as soon as you got access)
Once everything is green on Try, add the checkin-needed keyword to this bug.

::: js/src/asmjs/AsmJSValidate.cpp
@@ +5719,2 @@
>        case AsmJSSimdOperation_minNum:
> +        return CheckSimdBinary(f, call, opType, MSimdBinaryArith::minNum, def, type);

Now that both have the same case, I guess you can also use a macro as well.

::: js/src/jit/MCallOptimize.cpp
@@ +256,5 @@
>      if (native == js::CallOrConstructBoundFunction)
>          return inlineBoundFunction(callInfo, target);
>  
>      // Simd functions
> +#   define INLINE_INT32X4_SIMD_ARITH_(OP)                                                   \

nit: do not indent "define" and "undef"

::: js/src/jit/MIR.h
@@ +1958,5 @@
>      public MixPolicy<SimdSameAsReturnedTypePolicy<0>, SimdSameAsReturnedTypePolicy<1> >::Data
>  {
>    public:
>      enum Operation {
> +#       define OP_LIST_(OP) OP,

nit: same here.

Also, enumerated values are Capitalized by convention.  Maybe we should do something like:

# define OP_LIST_(OP) Op_ ## OP,
Attachment #8561544 - Flags: review?(nicolas.b.pierron) → review+
(Assignee)

Comment 10

3 years ago
Created attachment 8562113 [details] [diff] [review]
Macro
Attachment #8561544 - Attachment is obsolete: true
Attachment #8562113 - Flags: review+
(Assignee)

Comment 11

3 years ago
Comment on attachment 8562113 [details] [diff] [review]
Macro

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce23ebb46217

Try is green, so I'm setting the checkin flag. 
Thanks!
Attachment #8562113 - Flags: checkin?

Updated

3 years ago
Attachment #8562113 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/42a06f4f0de9
Status: UNCONFIRMED → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.