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

Hi, This patch inline the SIMD.Int32x4.or and SIMD.Int32x4.xor calls. Thanks :)

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

Hi! I wrote a MACRO to generate the conditions.

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?

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

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 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)

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

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 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!