In the fallible uint32 case this function still overrides the output register, this is bad for the baseline JIT, because there we need to keep R0 right.

Comment on attachment 707791 [details] [diff] [review] v1 Review of attachment 707791 [details] [diff] [review]: ----------------------------------------------------------------- This is Jan his territory, but I think I can review this ;). Looks fine. To be consistent with the loadFromTypedArray above, I've asked to change "scratch" to "temp". If you want to use scratch, that's fine too, but make sure all "loadFromTypedArray" functions use the same var name. ::: js/src/ion/IonMacroAssembler.cpp @@ +172,5 @@ > > template<typename T> > void > MacroAssembler::loadFromTypedArray(int arrayType, const T &src, const ValueOperand &dest, > + bool allowDouble, Register scratch, Label *fail) ..., Register temp, bool allowDouble, Label *fail) I would prefer the following order and also use "temp", as it is used for the other loadFromTypedArray function too. @@ +182,5 @@ > case TypedArray::TYPE_INT16: > case TypedArray::TYPE_UINT16: > case TypedArray::TYPE_INT32: > loadFromTypedArray(arrayType, src, AnyRegister(dest.scratchReg()), InvalidReg, NULL); > tagValue(JSVAL_TYPE_INT32, dest.scratchReg(), dest); Because the function provides a temp register, I would always use that instead of dest.scratchReg() now... So change it here and for following needs for a temp reg... @@ +210,5 @@ > } > break; > case TypedArray::TYPE_FLOAT32: > case TypedArray::TYPE_FLOAT64: > loadFromTypedArray(arrayType, src, AnyRegister(ScratchFloatReg), dest.scratchReg(), NULL); s/dest.scratchReg()/temp/ @@ +220,5 @@ > } > } > > template void MacroAssembler::loadFromTypedArray(int arrayType, const Address &src, const ValueOperand &dest, > + bool allowDouble, Register scratch, Label *fail); s/scratch/temp/ @@ +225,2 @@ > template void MacroAssembler::loadFromTypedArray(int arrayType, const BaseIndex &src, const ValueOperand &dest, > + bool allowDouble, Register scratch, Label *fail); s/scratch/temp/ ::: js/src/ion/IonMacroAssembler.h @@ +443,5 @@ > void loadFromTypedArray(int arrayType, const T &src, AnyRegister dest, Register temp, Label *fail); > > template<typename T> > void loadFromTypedArray(int arrayType, const T &src, const ValueOperand &dest, bool allowDouble, > + Register scratch, Label *fail); s/scratch/temp/

(In reply to Hannes Verschore [:h4writer] from comment #1) > Comment on attachment 707791 [details] [diff] [review] > v1 > > Review of attachment 707791 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is Jan his territory, but I think I can review this ;). Looks fine. > > To be consistent with the loadFromTypedArray above, I've asked to change > "scratch" to "temp". If you want to use scratch, that's fine too, but make > sure all "loadFromTypedArray" functions use the same var name. > > ::: js/src/ion/IonMacroAssembler.cpp > @@ +172,5 @@ > > > > template<typename T> > > void > > MacroAssembler::loadFromTypedArray(int arrayType, const T &src, const ValueOperand &dest, > > + bool allowDouble, Register scratch, Label *fail) > > ..., Register temp, bool allowDouble, Label *fail) > > I would prefer the following order and also use "temp", as it is used for > the other loadFromTypedArray function too. > > @@ +182,5 @@ > > case TypedArray::TYPE_INT16: > > case TypedArray::TYPE_UINT16: > > case TypedArray::TYPE_INT32: > > loadFromTypedArray(arrayType, src, AnyRegister(dest.scratchReg()), InvalidReg, NULL); > > tagValue(JSVAL_TYPE_INT32, dest.scratchReg(), dest); > > Because the function provides a temp register, I would always use that > instead of dest.scratchReg() now... So change it here and for following > needs for a temp reg... > I wouldn't do this here, because we avoid one move in tagValue. > @@ +210,5 @@ > > } > > break; > > case TypedArray::TYPE_FLOAT32: > > case TypedArray::TYPE_FLOAT64: > > loadFromTypedArray(arrayType, src, AnyRegister(ScratchFloatReg), dest.scratchReg(), NULL); > > s/dest.scratchReg()/temp/ Okay nice to have. > > @@ +220,5 @@ > > } > > } > > > > template void MacroAssembler::loadFromTypedArray(int arrayType, const Address &src, const ValueOperand &dest, > > + bool allowDouble, Register scratch, Label *fail); > > s/scratch/temp/ > > @@ +225,2 @@ > > template void MacroAssembler::loadFromTypedArray(int arrayType, const BaseIndex &src, const ValueOperand &dest, > > + bool allowDouble, Register scratch, Label *fail); > > s/scratch/temp/ > > ::: js/src/ion/IonMacroAssembler.h > @@ +443,5 @@ > > void loadFromTypedArray(int arrayType, const T &src, AnyRegister dest, Register temp, Label *fail); > > > > template<typename T> > > void loadFromTypedArray(int arrayType, const T &src, const ValueOperand &dest, bool allowDouble, > > + Register scratch, Label *fail); > > s/scratch/temp/ Thanks :)