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

Jump to bottom

cmd/compile: add special rewrite rule syntax for op renaming #36380

Closed
josharian opened this issue Jan 4, 2020 · 9 comments
Closed

cmd/compile: add special rewrite rule syntax for op renaming #36380

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

Comments

@josharian
Copy link
Contributor

A lot of rewrite rules are of the form:

(Op [c] a b) -> (LoweredOp [c] a b)

I propose we add a special form of rewrite rule for this common case. Something like:

(Op ...) -> (LoweredOp ...)

Or maybe * instead of ...? The rulegen engine would recognize this and generate just: v.Op = LoweredOp.

Upsides to having special machinery:

  • less generated code
  • no wasted work tearing down and recreating the same value
  • less opportunity for user error in spelling out every arg and aux and auxint (I recently had to debug an aux i forgot in just such a rule)

Opinions? I’d like to clear the design before writing the code.

cc @randall77 @mvdan
@randall77
Copy link
Contributor

I feel like I'd rather get rid of the Lowered* ops altogether.
The trick is specifying the register in/out for a generic op. But maybe we can find a way to do that instead?

@mvdan
Copy link
Member

mvdan commented Jan 4, 2020

Does this only affect lowering rules?

We could also consider doing this in parts. For example, we could recognise the cases where just the op changes, without touching the rule syntax at all.

@josharian
Copy link
Contributor Author

The trick is specifying the register in/out for a generic op. But maybe we can find a way to do that instead?

Seems do-able. For example, we could pull reg info out of opcodeData and instead give each arch a generic and a lowered reginfo table, and wire them up in config.go the way we do rewrite rules.

There are some other complications. For example, in regalloc, we check whether an op is generic for some wasm decisions about putting things on the stack. There may be other items like that that need to found and reworked.

We can do this incrementally, but it'll still create a lot of code churn, particularly in the rewrite rules.

We'd still have some op-only rewriting, but maybe not enough to care about. Example:

(Sub(64|32|16|8) x y) -> (SUB(Q|L|L|L) x y)

would become:

(Sub(16|8) x y) -> (Sub(32|32) x y)

@josharian
Copy link
Contributor Author

Does this only affect lowering rules?

No, but mostly.

For example, we could recognise the cases where just the op changes, without touching the rule syntax at all.

Not really. The semantics are different; a plain rule like this is actually zeroing+an op change. And detecting that nothing needs zeroing is hard. I actually tried this and decided that an explicit rule is way easier and cleaner.

@randall77
Copy link
Contributor

I wasn't suggesting that we not lower Sub64 to SUBQ.
I was thinking more for ops that don't generate code, or generate code only in $arch/ssa.go, like LoweredGetG or LoweredGetCallerSP.

@josharian
Copy link
Contributor Author

I was thinking more for ops that don't generate code, or generate code only in $arch/ssa.go, like LoweredGetG or LoweredGetCallerSP.

Ah. I apologize. I should have given as my initial example (A ...) -> (B ...). The LoweredX ops aren't that common among the kinds of rewrites I want to optimize here.

Here are the amd64 op-changing rewrite rules I am aware of. LoweredX ops are 6 out of 110.

OpAdd16:                  OpAMD64ADDL,
OpAdd32:                  OpAMD64ADDL,
OpAdd32F:                 OpAMD64ADDSS,
OpAdd64:                  OpAMD64ADDQ,
OpAdd64F:                 OpAMD64ADDSD,
OpAdd8:                   OpAMD64ADDL,
OpAddPtr:                 OpAMD64ADDQ,
OpAddr:                   OpAMD64LEAQ,
OpAnd16:                  OpAMD64ANDL,
OpAnd32:                  OpAMD64ANDL,
OpAnd64:                  OpAMD64ANDQ,
OpAnd8:                   OpAMD64ANDL,
OpAndB:                   OpAMD64ANDL,
OpAtomicAnd8:             OpAMD64ANDBlock,
OpAtomicCompareAndSwap32: OpAMD64CMPXCHGLlock,
OpAtomicCompareAndSwap64: OpAMD64CMPXCHGQlock,
OpAtomicLoad32:           OpAMD64MOVLatomicload,
OpAtomicLoad64:           OpAMD64MOVQatomicload,
OpAtomicLoad8:            OpAMD64MOVBatomicload,
OpAtomicLoadPtr:          OpAMD64MOVQatomicload,
OpAtomicOr8:              OpAMD64ORBlock,
OpAvg64u:                 OpAMD64AVGQU,
OpBswap32:                OpAMD64BSWAPL,
OpBswap64:                OpAMD64BSWAPQ,
OpClosureCall:            OpAMD64CALLclosure,
OpCom16:                  OpAMD64NOTL,
OpCom32:                  OpAMD64NOTL,
OpCom64:                  OpAMD64NOTQ,
OpCom8:                   OpAMD64NOTL,
OpConst16:                OpAMD64MOVLconst,
OpConst32:                OpAMD64MOVLconst,
OpConst32F:               OpAMD64MOVSSconst,
OpConst64:                OpAMD64MOVQconst,
OpConst64F:               OpAMD64MOVSDconst,
OpConst8:                 OpAMD64MOVLconst,
OpConstBool:              OpAMD64MOVLconst,
OpCtz16NonZero:           OpAMD64BSFL,
OpCtz32NonZero:           OpAMD64BSFL,
OpCtz8NonZero:            OpAMD64BSFL,
OpCvt32Fto32:             OpAMD64CVTTSS2SL,
OpCvt32Fto64:             OpAMD64CVTTSS2SQ,
OpCvt32Fto64F:            OpAMD64CVTSS2SD,
OpCvt32to32F:             OpAMD64CVTSL2SS,
OpCvt32to64F:             OpAMD64CVTSL2SD,
OpCvt64Fto32:             OpAMD64CVTTSD2SL,
OpCvt64Fto32F:            OpAMD64CVTSD2SS,
OpCvt64Fto64:             OpAMD64CVTTSD2SQ,
OpCvt64to32F:             OpAMD64CVTSQ2SS,
OpCvt64to64F:             OpAMD64CVTSQ2SD,
OpDiv128u:                OpAMD64DIVQU2,
OpDiv32F:                 OpAMD64DIVSS,
OpDiv64F:                 OpAMD64DIVSD,
OpGetCallerPC:            OpAMD64LoweredGetCallerPC,
OpGetCallerSP:            OpAMD64LoweredGetCallerSP,
OpGetClosurePtr:          OpAMD64LoweredGetClosurePtr,
OpGetG:                   OpAMD64LoweredGetG,
OpHmul32:                 OpAMD64HMULL,
OpHmul32u:                OpAMD64HMULLU,
OpHmul64:                 OpAMD64HMULQ,
OpHmul64u:                OpAMD64HMULQU,
OpInterCall:              OpAMD64CALLinter,
OpMul16:                  OpAMD64MULL,
OpMul32:                  OpAMD64MULL,
OpMul32F:                 OpAMD64MULSS,
OpMul64:                  OpAMD64MULQ,
OpMul64F:                 OpAMD64MULSD,
OpMul64uhilo:             OpAMD64MULQU2,
OpMul8:                   OpAMD64MULL,
OpNeg16:                  OpAMD64NEGL,
OpNeg32:                  OpAMD64NEGL,
OpNeg64:                  OpAMD64NEGQ,
OpNeg8:                   OpAMD64NEGL,
OpNilCheck:               OpAMD64LoweredNilCheck,
OpOr16:                   OpAMD64ORL,
OpOr32:                   OpAMD64ORL,
OpOr64:                   OpAMD64ORQ,
OpOr8:                    OpAMD64ORL,
OpOrB:                    OpAMD64ORL,
OpPopCount32:             OpAMD64POPCNTL,
OpPopCount64:             OpAMD64POPCNTQ,
OpRotateLeft16:           OpAMD64ROLW,
OpRotateLeft32:           OpAMD64ROLL,
OpRotateLeft64:           OpAMD64ROLQ,
OpRotateLeft8:            OpAMD64ROLB,
OpSignExt16to32:          OpAMD64MOVWQSX,
OpSignExt16to64:          OpAMD64MOVWQSX,
OpSignExt32to64:          OpAMD64MOVLQSX,
OpSignExt8to16:           OpAMD64MOVBQSX,
OpSignExt8to32:           OpAMD64MOVBQSX,
OpSignExt8to64:           OpAMD64MOVBQSX,
OpSqrt:                   OpAMD64SQRTSD,
OpStaticCall:             OpAMD64CALLstatic,
OpSub16:                  OpAMD64SUBL,
OpSub32:                  OpAMD64SUBL,
OpSub32F:                 OpAMD64SUBSS,
OpSub64:                  OpAMD64SUBQ,
OpSub64F:                 OpAMD64SUBSD,
OpSub8:                   OpAMD64SUBL,
OpSubPtr:                 OpAMD64SUBQ,
OpWB:                     OpAMD64LoweredWB,
OpXor16:                  OpAMD64XORL,
OpXor32:                  OpAMD64XORL,
OpXor64:                  OpAMD64XORQ,
OpXor8:                   OpAMD64XORL,
OpZeroExt16to32:          OpAMD64MOVWQZX,
OpZeroExt16to64:          OpAMD64MOVWQZX,
OpZeroExt32to64:          OpAMD64MOVLQZX,
OpZeroExt8to16:           OpAMD64MOVBQZX,
OpZeroExt8to32:           OpAMD64MOVBQZX,
OpZeroExt8to64:           OpAMD64MOVBQZX,

@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 7, 2020
@toothrot toothrot added this to the Backlog milestone Jan 7, 2020
@gopherbot
Copy link

Change https://golang.org/cl/213898 mentions this issue: cmd/compile: add ellipsis syntax for op-only rewrite rules

@josharian
Copy link
Contributor Author

It turned out to be easy to implement, so I whipped up CL 213898 as an experiment, to make this discussion concrete.

gopherbot pushed a commit that referenced this issue Feb 20, 2020
@josharian
cmd/compile: add ellipsis syntax for op-only rewrite rules
8484d40 
This change introduces a new syntax for rewrite rules
that only change a Value's Op. See #36380 for more discussion.

Updating rewrite rules to use ellipses will happen
in follow-up CLs.

Change-Id: I8c56e85de24607579d79729575c89ca80805ba5c
Reviewed-on: https://go-review.googlesource.com/c/go/+/213898
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
@josharian
Copy link
Contributor Author

This happened.

@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
Backlog
Development

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