Issue 6305100: code review 6305100: runtime: avoid r9/r10 during memmove (Closed)
|
Created:
12 years, 11 months ago by dave Modified:
12 years, 10 months ago Reviewers:
CC:
rsc, minux1, extraterrestrial, golang-dev Visibility:
Public. |
Descriptionruntime: avoid r9/r10 during memmove
Fixes issue 3718.
Requires CL 6300043.
Patch Set 1 #Patch Set 2 : code review 6305100: runtime: avoid r9/r10 during memmove #Patch Set 3 : diff -r 5e3661048f2e https://code.google.com/p/go #Patch Set 4 : diff -r 5e3661048f2e https://code.google.com/p/go #Patch Set 5 : diff -r 5e3661048f2e https://code.google.com/p/go #Patch Set 6 : diff -r 5e3661048f2e https://code.google.com/p/go #Patch Set 7 : diff -r 5e3661048f2e https://code.google.com/p/go #
Total comments: 4
Patch Set 8 : diff -r 5e3661048f2e https://code.google.com/p/go #
Total comments: 2
Patch Set 9 : diff -r f14046c7d922 https://code.google.com/p/go #Patch Set 10 : diff -r f14046c7d922 https://code.google.com/p/go #Patch Set 11 : diff -r 572efb136b1a https://go.googlecode.com/hg/ #MessagesTotal messages: 18
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
I really liked your other one where you shuffled things and used R0-R7. Is that not possible here?
Sign in to reply to this message.
I'll try to make that work. I might be able to place TS at 0 and TE at 8, then use R0-R7 and R1-R8 respectively. On Sun, Jun 17, 2012 at 9:56 AM, Russ Cox <rsc@golang.org> wrote: > I really liked your other one where you shuffled things and used > R0-R7. Is that not possible here?
Sign in to reply to this message.
I'm having a problem making this work
AND $~0x03, R(FROM) /* align source */
is being compiled into
0x000582f4 <+212>: mvn r11, #3
0x000582f8 <+216>: and r11, r11, r11
Which is trying to mask off the bottom two bits, but ends up putting
0xfffffffc into r11.
What am I doing wrong?
https://gist.github.com/2943532
Cheers
Dave
On Sun, Jun 17, 2012 at 10:03 AM, Dave Cheney <dave@cheney.net> wrote:
> I'll try to make that work. I might be able to place TS at 0 and TE at
> 8, then use R0-R7 and R1-R8 respectively.
>
> On Sun, Jun 17, 2012 at 9:56 AM, Russ Cox <rsc@golang.org> wrote:
>> I really liked your other one where you shuffled things and used
>> R0-R7. Is that not possible here?
Sign in to reply to this message.
On Sun, Jun 17, 2012 at 1:44 PM, Dave Cheney <dave@cheney.net> wrote: > I'm having a problem making this work > > AND $~0x03, R(FROM) /* align source */ > > is being compiled into > > 0x000582f4 <+212>: mvn r11, #3 > 0x000582f8 <+216>: and r11, r11, r11 > use "BIC $0x3, R(FROM)" directly. the linker is use r11 to synthesize certain instructions, and unfortunately we are also operating on r11 this time. i think 5l should give error for this case; but it doesn't try to use BIC which is a much better alternative.
Sign in to reply to this message.
Ahh, that makes sense and explains why this routine was so sensitive to reg layout. Previously r11 was the last bulk move reg, so it's contents we're not important. On 17/06/2012, at 16:42, minux <minux.ma@gmail.com> wrote: > > On Sun, Jun 17, 2012 at 1:44 PM, Dave Cheney <dave@cheney.net> wrote: > I'm having a problem making this work > > AND $~0x03, R(FROM) /* align source */ > > is being compiled into > > 0x000582f4 <+212>: mvn r11, #3 > 0x000582f8 <+216>: and r11, r11, r11 > use "BIC $0x3, R(FROM)" directly. > the linker is use r11 to synthesize certain instructions, and unfortunately we are > also operating on r11 this time. > > i think 5l should give error for this case; but it doesn't try to use BIC which is a > much better alternative.
Sign in to reply to this message.
On Sun, Jun 17, 2012 at 2:50 PM, Dave Cheney <dave@cheney.net> wrote: > Ahh, that makes sense and explains why this routine was so sensitive to > reg layout. Previously r11 was the last bulk move reg, so it's contents > we're not important. > probably we need a warning here: the linker will use r11 to synthesize certain instructions, so please double-check with objdump when using r11 directly.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, rsc@golang.org, minux.ma@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Performance appears unaffected, which is good BenchmarkMemmove32 20000000 114 ns/op 279.68 MB/s BenchmarkMemmove4K 1000000 1253 ns/op 3268.34 MB/s BenchmarkMemmove64K 50000 61538 ns/op 1064.97 MB/s BenchmarkMemmove4M 100 28315730 ns/op 148.13 MB/s BenchmarkMemmove64M 5 455950800 ns/op 147.18 MB/s
Sign in to reply to this message.
http://codereview.appspot.com/6305100/diff/10002/src/pkg/runtime/memmove_arm.s File src/pkg/runtime/memmove_arm.s (right): http://codereview.appspot.com/6305100/diff/10002/src/pkg/runtime/memmove_arm.... src/pkg/runtime/memmove_arm.s:26: TS = 0 /* TS or TE are spilled to the stack */ Please put multiline comments and definitions on different lines. Then we won't have to worry about alignment problems. http://codereview.appspot.com/6305100/diff/10002/src/pkg/runtime/memmove_arm.... src/pkg/runtime/memmove_arm.s:86: MOVW R(TS), 4(R13) /* save TS */ Does it work to say MOVW R(TS), savedts+0(SP) ? and similarly for the restore? It is supposed to. It might be 4 instead of 0 here but my first guess would be 0. Same for all the save/restores below.
Sign in to reply to this message.
Hello rsc@golang.org, minux.ma@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Would you like me to merge this CL into http://codereview.appspot.com/6300043/ as both are needed to resolve the issue ? On Mon, Jun 18, 2012 at 8:53 AM, <dave@cheney.net> wrote: > Hello rsc@golang.org, minux.ma@gmail.com (cc: > > golang-dev@googlegroups.com), > > Please take another look. > > > http://codereview.appspot.com/6305100/
Sign in to reply to this message.
LGTM It's fine as a separate CL. The review log is here.
Sign in to reply to this message.
Thanks. @minux, could I get a third pair of eyes on both of these CLs before committing? On 19/06/2012, at 14:10, rsc@golang.org wrote: > LGTM > > It's fine as a separate CL. The review log is here. > > > http://codereview.appspot.com/6305100/
Sign in to reply to this message.
http://codereview.appspot.com/6305100/diff/2004/src/pkg/runtime/memmove_arm.s File src/pkg/runtime/memmove_arm.s (right): http://codereview.appspot.com/6305100/diff/2004/src/pkg/runtime/memmove_arm.s... src/pkg/runtime/memmove_arm.s:30: // Warning: the linker will use R11 to syntehsize certain instructions. Please s/syntehsize/synthesize
Sign in to reply to this message.
Hello rsc@golang.org, minux.ma@gmail.com, extraterrestrial.neighbour@googlemail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Thank you for your comments, PTAL. http://codereview.appspot.com/6305100/diff/10002/src/pkg/runtime/memmove_arm.s File src/pkg/runtime/memmove_arm.s (right): http://codereview.appspot.com/6305100/diff/10002/src/pkg/runtime/memmove_arm.... src/pkg/runtime/memmove_arm.s:26: TS = 0 /* TS or TE are spilled to the stack */ On 2012/06/17 14:50:17, rsc wrote: > Please put multiline comments and definitions on different lines. > Then we won't have to worry about alignment problems. Done. http://codereview.appspot.com/6305100/diff/10002/src/pkg/runtime/memmove_arm.... src/pkg/runtime/memmove_arm.s:86: MOVW R(TS), 4(R13) /* save TS */ On 2012/06/17 14:50:17, rsc wrote: > Does it work to say > > MOVW R(TS), savedts+0(SP) > > ? > > and similarly for the restore? It is supposed to. It might be 4 instead of 0 > here but my first guess would be 0. > > Same for all the save/restores below. Done. http://codereview.appspot.com/6305100/diff/2004/src/pkg/runtime/memmove_arm.s File src/pkg/runtime/memmove_arm.s (right): http://codereview.appspot.com/6305100/diff/2004/src/pkg/runtime/memmove_arm.s... src/pkg/runtime/memmove_arm.s:30: // Warning: the linker will use R11 to syntehsize certain instructions. Please On 2012/06/19 16:29:45, extraterrestrial wrote: > s/syntehsize/synthesize Done.
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=5ca8acc84025 *** runtime: avoid r9/r10 during memmove Fixes issue 3718. Requires CL 6300043. R=rsc, minux.ma, extraterrestrial.neighbour CC=golang-dev http://codereview.appspot.com/6305100
Sign in to reply to this message.
|
