IonMonkey: Don't clobber out register in loadFromTypedArray

RESOLVED FIXED in mozilla21

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

Trunk
mozilla21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Description

7 years ago
Posted patch v1Splinter Review
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.
Attachment #707791 - Flags: review?(hv1989)
Assignee

Updated

7 years ago
Blocks: 836005
Assignee

Updated

7 years ago
Assignee: general → evilpies
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
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/
Attachment #707791 - Flags: review?(hv1989) → review+
Assignee

Comment 2

7 years ago
(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 :)
Assignee

Updated

7 years ago
Summary: IonMonkey: Don't clober out register in loadFromTypedArray → IonMonkey: Don't clobber out register in loadFromTypedArray
https://hg.mozilla.org/mozilla-central/rev/de2b4e74861f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.