Closed Bug 1129148 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: victorcarlquist, Assigned: victorcarlquist)

References

Details

Attachments

(1 file, 4 obsolete files)

We need to inline the SIMD.int32x4.or and SIMD.int32x4.xor calls.
Blocks: 1112155
Depends on: 1127929
Attached patch Patch (obsolete) — Splinter Review
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+
Attached patch Macro (obsolete) — Splinter Review
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?
Attached patch Macro (obsolete) — Splinter Review
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+
Attached patch Macro (obsolete) — Splinter Review
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+
Attached patch MacroSplinter Review
Attachment #8561544 - Attachment is obsolete: true
Attachment #8562113 - Flags: review+
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?
Attachment #8562113 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/42a06f4f0de9
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.