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: add special rewrite rule syntax for op renaming #36380

Open
josharian opened this issue Jan 4, 2020 · 8 comments
Open

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

josharian opened this issue Jan 4, 2020 · 8 comments
Milestone

Comments

@josharian
Copy link
Contributor

@josharian josharian commented Jan 4, 2020

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

This comment has been minimized.

Copy link
Contributor

@randall77 randall77 commented Jan 4, 2020

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

This comment has been minimized.

Copy link
Member

@mvdan 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

This comment has been minimized.

Copy link
Contributor Author

@josharian josharian commented Jan 4, 2020

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

This comment has been minimized.

Copy link
Contributor Author

@josharian josharian commented Jan 4, 2020

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

This comment has been minimized.

Copy link
Contributor

@randall77 randall77 commented Jan 5, 2020

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

This comment has been minimized.

Copy link
Contributor Author

@josharian josharian commented Jan 5, 2020

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 this to the Backlog milestone Jan 7, 2020
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 8, 2020

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

@josharian

This comment has been minimized.

Copy link
Contributor Author

@josharian josharian commented Jan 8, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.