Switch memset.arm.S to unified syntax.

Switch memset.arm.S to unified syntax.

BUG=124610
TEST=None

[eugenis, 2012-04-25 06:22:39]

ping

[thakis, 2012-04-30 21:26:36]

Sorry for the delay, rubber-stampy LGTM

I'm not familiar with ARM assembly. I read 

http://sourceware.org/binutils/docs/as/ARM_002dInstruction_002dSet.html#ARM_0...
and skimmed
http://web.eecs.umich.edu/~prabal/teaching/eecs373-f10/readings/ARMv7-M_ARM.pdf
. If that's good enough for you, feel free to land, else you might want to ask
someone who's familiar with arm assembly.

https://codereview.appspot.com/6108046/diff/1/opts/memset.arm.S
File opts/memset.arm.S (right):

https://codereview.appspot.com/6108046/diff/1/opts/memset.arm.S#newcode45
opts/memset.arm.S:45: orr         r1, r1, r1, lsl #16
Why do you need r1 twice here? According to the ORR description in
http://web.eecs.umich.edu/~prabal/teaching/eecs373-f10/readings/ARMv7-M_ARM.pdf
it's dest reg, source reg, intermediate

[TomH, 2012-04-30 21:32:56]

I pinged djsollen@; he's probably the best reviewier on the Skia team for this.

[thakis, 2012-04-30 21:38:36]

https://codereview.appspot.com/6108046/diff/1/opts/memset.arm.S
File opts/memset.arm.S (right):

https://codereview.appspot.com/6108046/diff/1/opts/memset.arm.S#newcode45
opts/memset.arm.S:45: orr         r1, r1, r1, lsl #16
On 2012/04/30 21:26:36, thakis wrote:
> Why do you need r1 twice here? According to the ORR description in
>
http://web.eecs.umich.edu/%7Eprabal/teaching/eecs373-f10/readings/ARMv7-M_ARM...
> it's dest reg, source reg, intermediate

s/twice/thrice/

[DerekS, 2012-05-01 12:30:55]

On 2012/04/30 21:38:36, thakis wrote:
> https://codereview.appspot.com/6108046/diff/1/opts/memset.arm.S
> File opts/memset.arm.S (right):
> 
> https://codereview.appspot.com/6108046/diff/1/opts/memset.arm.S#newcode45
> opts/memset.arm.S:45: orr         r1, r1, r1, lsl #16
> On 2012/04/30 21:26:36, thakis wrote:
> > Why do you need r1 twice here? According to the ORR description in
> >
>
http://web.eecs.umich.edu/%257Eprabal/teaching/eecs373-f10/readings/ARMv7-M_A...
> > it's dest reg, source reg, intermediate
> 
> s/twice/thrice/

Can you justify the need for the additional r1 param?  Other than that I think
this CL looks fine to me as it just switches to a different syntax.

[eugenis, 2012-05-02 09:12:37]

On 2012/05/01 12:30:55, djsollen wrote:
> On 2012/04/30 21:38:36, thakis wrote:
> > https://codereview.appspot.com/6108046/diff/1/opts/memset.arm.S
> > File opts/memset.arm.S (right):
> > 
> > https://codereview.appspot.com/6108046/diff/1/opts/memset.arm.S#newcode45
> > opts/memset.arm.S:45: orr         r1, r1, r1, lsl #16
> > On 2012/04/30 21:26:36, thakis wrote:
> > > Why do you need r1 twice here? According to the ORR description in
> > >
> >
>
http://web.eecs.umich.edu/%25257Eprabal/teaching/eecs373-f10/readings/ARMv7-M...
> > > it's dest reg, source reg, intermediate
> > 
> > s/twice/thrice/
> 
> Can you justify the need for the additional r1 param?  Other than that I think
> this CL looks fine to me as it just switches to a different syntax.

I think you are reading docs for the wrong ORR. This one is ORR (register) in
A6.7.91 of the same document. According to the comment on page A6-175, if shift
(lsl #16) is specified, only encoding T2 (with 3 registers) is allowed.

[DerekS, 2012-05-02 11:49:53]

On 2012/05/02 09:12:37, eugenis wrote:
> On 2012/05/01 12:30:55, djsollen wrote:
> > On 2012/04/30 21:38:36, thakis wrote:
> > > https://codereview.appspot.com/6108046/diff/1/opts/memset.arm.S
> > > File opts/memset.arm.S (right):
> > > 
> > > https://codereview.appspot.com/6108046/diff/1/opts/memset.arm.S#newcode45
> > > opts/memset.arm.S:45: orr         r1, r1, r1, lsl #16
> > > On 2012/04/30 21:26:36, thakis wrote:
> > > > Why do you need r1 twice here? According to the ORR description in
> > > >
> > >
> >
>
http://web.eecs.umich.edu/%2525257Eprabal/teaching/eecs373-f10/readings/ARMv7...
> > > > it's dest reg, source reg, intermediate
> > > 
> > > s/twice/thrice/
> > 
> > Can you justify the need for the additional r1 param?  Other than that I
think
> > this CL looks fine to me as it just switches to a different syntax.
> 
> I think you are reading docs for the wrong ORR. This one is ORR (register) in
> A6.7.91 of the same document. According to the comment on page A6-175, if
shift
> (lsl #16) is specified, only encoding T2 (with 3 registers) is allowed.

LGTM

[eugenis, 2012-05-02 13:02:04]

On 2012/05/02 11:49:53, djsollen wrote:
> On 2012/05/02 09:12:37, eugenis wrote:
> > On 2012/05/01 12:30:55, djsollen wrote:
> > > On 2012/04/30 21:38:36, thakis wrote:
> > > > https://codereview.appspot.com/6108046/diff/1/opts/memset.arm.S
> > > > File opts/memset.arm.S (right):
> > > > 
> > > >
https://codereview.appspot.com/6108046/diff/1/opts/memset.arm.S#newcode45
> > > > opts/memset.arm.S:45: orr         r1, r1, r1, lsl #16
> > > > On 2012/04/30 21:26:36, thakis wrote:
> > > > > Why do you need r1 twice here? According to the ORR description in
> > > > >
> > > >
> > >
> >
>
http://web.eecs.umich.edu/%252525257Eprabal/teaching/eecs373-f10/readings/ARM...
> > > > > it's dest reg, source reg, intermediate
> > > > 
> > > > s/twice/thrice/
> > > 
> > > Can you justify the need for the additional r1 param?  Other than that I
> think
> > > this CL looks fine to me as it just switches to a different syntax.
> > 
> > I think you are reading docs for the wrong ORR. This one is ORR (register)
in
> > A6.7.91 of the same document. According to the comment on page A6-175, if
> shift
> > (lsl #16) is specified, only encoding T2 (with 3 registers) is allowed.
> 
> LGTM

Thanks. I'm not committer in skia, could you land this please?

[DerekS, 2012-05-02 14:16:09]

Committed revision 3816. You can now close this issue.

[eugenis, 2012-05-02 14:17:10]

On 2012/05/02 14:16:09, djsollen wrote:
> Committed revision 3816. You can now close this issue.

Thanks!