Skip to content

cmd/compile: ssa rewrite x-(x&y) => x&^y #52665

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

Closed
wants to merge 1 commit into from
Closed

Conversation

Jorropo
Copy link
Member

@Jorropo Jorropo commented May 3, 2022

This allows to rewrite:
AND BX, AX
SUB AX, BX

Into (with GOAMD64=v3):
ANDN AX, BX, AX

Which is twice as fast, and smaller.

Into (without GOAMD64=v3):
NOT BX
AND AX, BX

Which if AX's depency isn't resolved yet but BX is in most cases twice as
fast because now the NOT BX and AX's depency can execute in parallel.
In other cases, it has negligeable positive impact.

Other instructions set will also benefits from the pipelining adventage.
And also benefit from the smaller ANDN instruction if they have something
similar.

Help reduce ChaCha20 and Poly1305 to #52563

@gopherbot
Copy link
Contributor

This PR (HEAD: 36dddc1) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/403654 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

This allows to rewrite:
  AND BX, AX
  SUB AX, BX

Into (with GOAMD64=v3):
  ANDN AX, BX, AX

Which is twice as fast, and smaller.

Into (without GOAMD64=v3):
  NOT BX
  AND AX, BX

Which if AX's depency isn't resolved yet but BX is in most cases twice as
fast because now the NOT BX and AX's depency can execute in parallel.
In other cases, it has negligeable positive impact.

Other instructions set will also benefits from the pipelining adventage.
And also benefit from the smaller ANDN instruction if they have something
similar.

Help reduce ChaCha20 and Poly1305 to golang#52563
@gopherbot
Copy link
Contributor

This PR (HEAD: a01fd33) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/403654 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Keith Randall:

Patch Set 2:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/403654.
After addressing review feedback, remember to publish your drafts!

@greatroar
Copy link

greatroar commented May 4, 2022

Are you sure the separate rule for amd64 is needed? It looks redundant given the generic rule. NOT(L|Q) isn't generated for much else than Com(8|16|32|64).

Also, ChaPoly has constant y, so I expect that would be an AND(L|Q)const by the time that rule is checked.

@Jorropo
Copy link
Member Author

Jorropo commented May 4, 2022

Are you sure the separate rule for amd64 is needed? It looks redundant given the generic rule.

@greatroar

Good point, this is a remanent from my first patch which was:

(Sub(64|32|16|8) x z:(And(64|32|16|8) <t> x y)) && z.Uses == 1 => (And(64|32|16|8) x (Com(64|32|16|8) <t> y))

To avoid generating extra instructions where we wouldn't need them.
Turns out, this is not needed, will fix.

NOT(L|Q) isn't generated for much else than Com(8|16|32|64).

I don't understand that, I do not emit or match any NOT outside of the Com in the generic.
The (AND x (NOT y)) rule is one that already exists.

Also, ChaPoly has constant y, so I expect that would be an AND(L|Q)const by the time that rule is checked.

That why the generic rule exists. 🙃

@greatroar
Copy link

I don't understand that, I do not emit or match any NOT outside of the Com in the generic.

Never mind, brain fart.

@Jorropo
Copy link
Member Author

Jorropo commented May 5, 2022

This is premature, should be done once the BCE issue is fixed first and we can see a real improvement in ChaChaPoly.

@Jorropo Jorropo closed this May 5, 2022
@Jorropo Jorropo deleted the ANDN branch May 5, 2022 13:30
@Jorropo Jorropo restored the ANDN branch May 5, 2022 13:30
@Jorropo Jorropo deleted the ANDN branch May 5, 2022 13:30
@Jorropo Jorropo restored the ANDN branch May 5, 2022 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants