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

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Victor Carlquist]

We need to inline the SIMD.int32x4.or and SIMD.int32x4.xor calls.

Comment 1

[Victor Carlquist]

Attachment: Patch

Hi, 

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

Thanks :)

Comment 2

[Nicolas B. Pierron [:nbp]]

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

Comment 3

[Victor Carlquist]

Attachment: Macro

Hi!
I wrote a MACRO to generate the conditions.

Comment 4

[Nicolas B. Pierron [:nbp]]

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?

Comment 5

[Victor Carlquist]

Attachment: Macro

I needed to change the enum in MSimdArith to simplify the MACRO.

Comment 6

[Nicolas B. Pierron [:nbp]]

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.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

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)

Comment 8

[Victor Carlquist]

Attachment: Macro

Hi, I made the changes that you and Benjamin suggested :)

Comment 9

[Nicolas B. Pierron [:nbp]]

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,

Comment 10

[Victor Carlquist]

Attachment: Macro

Comment 11

[Victor Carlquist]

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!

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/42a06f4f0de9

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/42a06f4f0de9