|
|
Created:
12 years, 4 months ago by eugenis Modified:
12 years, 4 months ago CC:
skia-review_googlegroups.com Base URL:
http://skia.googlecode.com/svn/trunk/src/ Visibility:
Public. |
DescriptionSwitch memset.arm.S to unified syntax.
BUG=124610
TEST=None
Patch Set 1 #
Total comments: 2
MessagesTotal messages: 11
ping
Sign in to reply to this message.
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
Sign in to reply to this message.
I pinged djsollen@; he's probably the best reviewier on the Skia team for this.
Sign in to reply to this message.
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/
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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?
Sign in to reply to this message.
Committed revision 3816. You can now close this issue.
Sign in to reply to this message.
On 2012/05/02 14:16:09, djsollen wrote: > Committed revision 3816. You can now close this issue. Thanks!
Sign in to reply to this message.
|