code review 9040043: cmd/6c, cmd/8c: avoid generating long multiplication wh...

cmd/6c, cmd/8c: avoid generating long multiplication when not necessary.

On amd64
benchmark                        old ns/op    new ns/op    delta
BenchmarkHashStringSpeed                91           74  -18.49%
BenchmarkHashInt32Speed                 54           45  -17.88%
BenchmarkHashInt64Speed                 76           58  -23.53%
BenchmarkHashStringArraySpeed          231          188  -18.61%

Fixes issue 5367.

[remyoudompheng, 2013-05-04 11:12:47]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[iant, 2013-05-04 16:09:40]

My inclination would be to hold this until after 1.1.

[dfc, 2013-05-15 00:26:12]

Ping. The change window has been well and truly thrown open.

On Sun, May 5, 2013 at 2:09 AM,  <iant@golang.org> wrote:
> My inclination would be to hold this until after 1.1.
>
>
> https://codereview.appspot.com/9040043/
>
>
> --
>
> ---You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[iant, 2013-05-15 13:45:29]

I like the idea but frankly I have no idea what this change is doing.  Anybody
out there understand the C compilers?

[DMorsing, 2013-05-15 13:49:31]

On 2013/05/15 13:45:29, iant wrote:
> I like the idea but frankly I have no idea what this change is doing.  Anybody
> out there understand the C compilers?

Looks like it's bumping the usage count for the DX register so that it is not
chosen when register allocating, but I have no clue how that translates to a
speed improvement.

[remyoudompheng, 2013-05-15 14:15:24]

gopcode does logic on usage count of DX to do short or long
multiplication. (weird)

2013/5/15, daniel.morsing@gmail.com <daniel.morsing@gmail.com>:
> On 2013/05/15 13:45:29, iant wrote:
>> I like the idea but frankly I have no idea what this change is doing.
> Anybody
>> out there understand the C compilers?
>
> Looks like it's bumping the usage count for the DX register so that it
> is not chosen when register allocating, but I have no clue how that
> translates to a speed improvement.
>
> https://codereview.appspot.com/9040043/
>

[ality, 2013-05-15 14:32:53]

Rémy,

Shouldn't you instead duplicate the gopcode AX/DX logic for the OASLMUL/OLMUL
case? With your change OLMUL nodes now generate IMUL
opcodes instead of MUL.

[ality, 2013-05-15 14:50:59]

To restate my question: doesn't this effectively turn unsigned multiplication
into signed?

[remyoudompheng, 2013-05-15 14:59:55]

What do you mean by signed multiplication? It would only make sense
for the long one.

2013/5/15, ality@pbrane.org <ality@pbrane.org>:
> To restate my question: doesn't this effectively turn unsigned
> multiplication into signed?
>
> https://codereview.appspot.com/9040043/
>

[ality, 2013-05-15 15:22:02]

On 2013/05/15 14:59:55, remyoudompheng wrote:
> What do you mean by signed multiplication? It would only make sense
> for the long one.

OLMUL is for multiplying unsigned types, right? tcom checks typeu and
changes OMUL to OLMUL if needed. Basically, I'm asking if you should
do gopcode(o, ...) instead of gopcode(OMUL, ...) after incrementing
reg[D_DX].

[remyoudompheng, 2013-05-18 08:19:56]

On 2013/05/15 15:22:02, ality wrote:
> On 2013/05/15 14:59:55, remyoudompheng wrote:
> > What do you mean by signed multiplication? It would only make sense
> > for the long one.
> 
> OLMUL is for multiplying unsigned types, right? tcom checks typeu and
> changes OMUL to OLMUL if needed. Basically, I'm asking if you should
> do gopcode(o, ...) instead of gopcode(OMUL, ...) after incrementing
> reg[D_DX].

OLMUL is mapped to MUL and OMUL is mapped to IMUL. MUL doesn't have a short form
(since ordinary multiplication is independent of signedness).

[dfc, 2013-05-18 08:21:02]

> OLMUL is mapped to MUL and OMUL is mapped to IMUL

That is not confusing in the slightest.

On Sat, May 18, 2013 at 6:19 PM,  <remyoudompheng@gmail.com> wrote:
> On 2013/05/15 15:22:02, ality wrote:
>>
>> On 2013/05/15 14:59:55, remyoudompheng wrote:
>> > What do you mean by signed multiplication? It would only make sense
>> > for the long one.
>
>
>> OLMUL is for multiplying unsigned types, right? tcom checks typeu and
>> changes OMUL to OLMUL if needed. Basically, I'm asking if you should
>> do gopcode(o, ...) instead of gopcode(OMUL, ...) after incrementing
>> reg[D_DX].
>
>
> OLMUL is mapped to MUL and OMUL is mapped to IMUL. MUL doesn't have a
> short form (since ordinary multiplication is independent of signedness).
>
> https://codereview.appspot.com/9040043/

[dfc, 2013-05-21 12:00:53]

ping.

[dfc, 2013-05-27 22:05:42]

On 2013/05/21 12:00:53, dfc wrote:
> ping.

ping

[rsc, 2013-06-03 19:43:14]

LGTM

https://codereview.appspot.com/9040043/diff/5001/src/cmd/6c/cgen.c
File src/cmd/6c/cgen.c (right):

https://codereview.appspot.com/9040043/diff/5001/src/cmd/6c/cgen.c#newcode401
src/cmd/6c/cgen.c:401: reg[D_DX]++; // avoid doing long multiplication.
Please change this comment to

// for gopcode case OMUL

[remyoudompheng, 2013-06-04 06:34:36]

*** Submitted as https://code.google.com/p/go/source/detail?r=1c764773c6ce ***

cmd/6c, cmd/8c: avoid generating long multiplication when not necessary.

On amd64
benchmark                        old ns/op    new ns/op    delta
BenchmarkHashStringSpeed                91           74  -18.49%
BenchmarkHashInt32Speed                 54           45  -17.88%
BenchmarkHashInt64Speed                 76           58  -23.53%
BenchmarkHashStringArraySpeed          231          188  -18.61%

Fixes issue 5367.

R=golang-dev, iant, dave, daniel.morsing, ality, rsc
CC=golang-dev
https://codereview.appspot.com/9040043

https://codereview.appspot.com/9040043/diff/5001/src/cmd/6c/cgen.c
File src/cmd/6c/cgen.c (right):

https://codereview.appspot.com/9040043/diff/5001/src/cmd/6c/cgen.c#newcode401
src/cmd/6c/cgen.c:401: reg[D_DX]++; // avoid doing long multiplication.
On 2013/06/03 19:43:15, rsc wrote:
> Please change this comment to
> 
> // for gopcode case OMUL
> 

Done.