Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/compile: Could make better use of "bimm64" on ARM64 #19857

Closed
dr2chase opened this issue Apr 5, 2017 · 5 comments

Comments

Projects
None yet
3 participants
@dr2chase
Copy link
Contributor

commented Apr 5, 2017

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

1.9devel

What operating system and processor architecture are you using (go env)?

ARM64

What did you do?

Compiled x & ((1<<63)-1)

We currently convert this to a
(LSR 1 (LSL 1 x))
because that is more efficient than loading the immediate.

However, 64-bit AND apparently takes a "bimm64" operand which can contain some not entirely well specified runs of 0 and 1, that happens to include ((1<<63)-1). This came from an investigation into whether the problematic idiom described in #19809 might occur in code compiled from C or C++. I discovered that it does not, and that the operand can be expressed compactly as an immediate to logical operations.

I don't yet know if the Go assembler handles bimm64; the bug is filed so as not to forget this.

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2017

Generic rule rewrites certain AND to shifts

// Rewrite AND of consts as shifts if possible, slightly faster for 64 bit operands
// leading zeros can be shifted left, then right
(And64 <t> (Const64 [y]) x) && nlz(y) + nto(y) == 64 && nto(y) >= 32
  -> (Rsh64Ux64 (Lsh64x64 <t> x (Const64 <t> [nlz(y)])) (Const64 <t> [nlz(y)]))

and we didn't undo this in ARM64 rules. Should we?

@dr2chase

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2017

We could, it would be easy, just augment the (new) rule that converts (LSR k(LSL k x)) into (ROR k (LSL k x)) into one that uses the AND. I think no need to check the usage of the LSL because if it's > 1 that means the instruction count remains the same.

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2017

Yeah, I did this. It also reveals a (performance) bug in the assembler backend that some constants did not get encoded into instruction although it could. CLs are coming.

@gopherbot

This comment has been minimized.

Copy link

commented Apr 6, 2017

CL https://golang.org/cl/39651 mentions this issue.

@gopherbot

This comment has been minimized.

Copy link

commented Apr 6, 2017

CL https://golang.org/cl/39652 mentions this issue.

@gopherbot gopherbot closed this in 257b01f Apr 6, 2017

gopherbot pushed a commit that referenced this issue Apr 6, 2017

cmd/internal/obj/arm64: fix encoding of AND MBCON
When a constant is both MOVCON (can fit into a MOV instruction)
and BITCON (can fit into a logical instruction), the assembler
chooses to use the MOVCON encoding, which is actually longer for
logical instructions. We add MBCON rules explicitly to make sure
it uses the BITCON encoding.

Updates #19857.

Change-Id: Ib9881be363cbc491ac2a0792b36b87e74eff34a8
Reviewed-on: https://go-review.googlesource.com/39652
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>

lparth added a commit to lparth/go that referenced this issue Apr 13, 2017

cmd/compile: use ANDconst to mask out leading/trailing bits on ARM64
For an AND that masks out leading or trailing bits, generic rules
rewrite it to a pair of shifts. On ARM64, the mask actually can
fit into an AND instruction. So we rewrite it back to AND.

Fixes golang#19857.

Change-Id: I479d7320ae4f29bb3f0056d5979bde4478063a8f
Reviewed-on: https://go-review.googlesource.com/39651
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>

lparth added a commit to lparth/go that referenced this issue Apr 13, 2017

cmd/internal/obj/arm64: fix encoding of AND MBCON
When a constant is both MOVCON (can fit into a MOV instruction)
and BITCON (can fit into a logical instruction), the assembler
chooses to use the MOVCON encoding, which is actually longer for
logical instructions. We add MBCON rules explicitly to make sure
it uses the BITCON encoding.

Updates golang#19857.

Change-Id: Ib9881be363cbc491ac2a0792b36b87e74eff34a8
Reviewed-on: https://go-review.googlesource.com/39652
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>

@golang golang locked and limited conversation to collaborators Apr 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.