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.

Sign up for GitHub

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

Jump to bottom

cmd/compile: support named values on RHS of rewrite rules #38621

Closed
josharian opened this issue Apr 23, 2020 · 4 comments
Closed

cmd/compile: support named values on RHS of rewrite rules #38621

josharian opened this issue Apr 23, 2020 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@josharian
Copy link
Contributor

I'd like to change the amd64 splitload rules to use (TESTQ x x) instead of (CMPQconst [0] x) when possible. However, there's no way to write (TESTQ x x) on the RHS of a rewrite rule unless x is already present on the LHS. You can spell out x twice, but then it generates two distinct values. In normal rewrite rules, we rely on a subsequent CSE to clean that up. But no CSE runs after splitload. Plus that is inefficient. I'd like to be able to write something like:

(CMP(Q|L|W|B)constload {sym} [vo] ptr mem) && valOnly(vo) == 0 -> (TEST(Q|L|W|B) x:(MOV(Q|L|W|B)load {sym} [offOnly(vo)] ptr mem) x)

Comments?

cc @mvdan @randall77
@mvdan
Copy link
Member

mvdan commented Apr 23, 2020

How does this compare to #37423? Both seem to want to add syntax for similar reasons.

@josharian
Copy link
Contributor Author

#37423 contemplates calculating reusable Go values with Go expressions. This contemplates generating reusable ssa.Values with s-exprs.

@randall77
Copy link
Contributor

That syntax seems fine to me. We already do it on the LHS.

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 23, 2020
@andybons andybons added this to the Unplanned milestone Apr 23, 2020
@gopherbot
Copy link

Change https://golang.org/cl/229701 mentions this issue: cmd/compile: allow named values on RHS of rewrite rules

xujianhai666 pushed a commit to xujianhai666/go-1 that referenced this issue May 21, 2020
@josharian @xujianhai666
cmd/compile: allow named values on RHS of rewrite rules
65f6f71 
Fixes golang#38621

Change-Id: Idbffdcc70903290dc58e5abb4867718bd5449fe1
Reviewed-on: https://go-review.googlesource.com/c/go/+/229701
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
@golang golang locked and limited conversation to collaborators Apr 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@josharian @andybons @mvdan @randall77 @gopherbot