Skip to content

Commit

Permalink
cmd/compile/internal: eliminate special branch psuedo-ops
Browse files Browse the repository at this point in the history 
The behavior of the RISCV BRANCH block is not safe. It reaches into the
control value and uses the registers of the control's args, rather than
the control value itself.

regalloc ensures that the control value is in a register, but it does
*not* do so for the control's arguments. It does not know that BRANCH
uses them and thus may not consider them live.

The reason for this weird BRANCH behavior is that RISC-V branches are
binary instructions, comparing two operands to decide which branch to
take, but we only have one control value, not two.

The best way to solve this would be to teach the compiler than some
blocks need two control values. While that doesn't look too difficult to
implement, it is a fairly large scale, intrusive change to the compiler.
Maintaining these changes as upstream continues to change the compiler
would become a painful maintainance burden.

Instead, make the branches behave much more like other architectures,
where the condition comparision is done in an instruction before the
branch. We then generate a branch instruction that simply compares the
condition with zero.

Once we are merged upstream, we can come back and investigate generating
better code by adding a second control value.

Fixes golang#12

Change-Id: I926d7ea45973c88927d42e915eb68265e79eb345
  • Loading branch information
@prattmic
prattmic committed Nov 30, 2016
1 parent 98556aa commit 8abd4ac
Show file tree
Hide file tree
Showing 5 changed files with 20 additions and 360 deletions.
33 changes: 5 additions & 28 deletions src/cmd/compile/internal/riscv/ssa.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -340,10 +340,6 @@ func ssaGenValue(s *gc.SSAGenState, v *ssa.Value) {
p.From.Reg = v.Args[0].Reg()
p.To.Type = obj.TYPE_REG
p.To.Reg = v.Reg()
case ssa.OpRISCVBEQ, ssa.OpRISCVBNE, ssa.OpRISCVBLT, ssa.OpRISCVBLTU, ssa.OpRISCVBGE, ssa.OpRISCVBGEU:
// These are flag pseudo-ops used as control values for conditional branch blocks.
// See the discussion in RISCVOps.
// The actual conditional branch instruction will be issued in ssaGenBlock.
case ssa.OpRISCVCALLstatic, ssa.OpRISCVCALLclosure, ssa.OpRISCVCALLdefer, ssa.OpRISCVCALLgo, ssa.OpRISCVCALLinter:
if v.Op == ssa.OpRISCVCALLstatic && v.Aux.(*gc.Sym) == gc.Deferreturn.Sym {
// Deferred calls will appear to be returning to
Expand Down Expand Up @@ -513,32 +509,13 @@ func ssaGenBlock(s *gc.SSAGenState, b, next *ssa.Block) {
p.To.Type = obj.TYPE_MEM
p.To.Name = obj.NAME_EXTERN
p.To.Sym = gc.Linksym(b.Aux.(*gc.Sym))
case ssa.BlockRISCVBRANCH:
// Conditional branch. The control value tells us what kind.
v := b.Control

// The control value need not be in this block, as the code
// below asserts. regalloc will ensure that it is in a register
// at this point so we can use it.
//
// FIXME(prattmic): Remove this block. It is left for now for
// easy debugging should the assertion above turn out to be
// incorrect after all.
if false {
// Double-check that the value is exactly where we expect it.
if v.Block != b {
gc.Fatalf("control value in the wrong block %v, want %v: %v", v.Block, b, v.LongString())
}
if v != b.Values[len(b.Values)-1] {
gc.Fatalf("badly scheduled control value for block %v: %v", b, v.LongString())
}
}

p := gc.Prog(v.Op.Asm())
case ssa.BlockRISCVBNE:
// Conditional branch if Control != 0.
p := gc.Prog(riscv.ABNE)
p.To.Type = obj.TYPE_BRANCH
p.Reg = v.Args[1].Reg()
p.Reg = b.Control.Reg()
p.From.Type = obj.TYPE_REG
p.From.Reg = v.Args[0].Reg()
p.From.Reg = riscv.REG_ZERO
switch next {
case b.Succs[0].Block():
p.As = riscv.InvertBranch(p.As)
Expand Down
24 changes: 8 additions & 16 deletions src/cmd/compile/internal/ssa/gen/RISCV.rules
View file
Original file line number Diff line number Diff line change
Expand Up @@ -422,22 +422,14 @@
(Addr {sym} base) -> (MOVaddr {sym} base)

// Conditional branches
(If (Eq64 x y) yes no) -> (BRANCH (BEQ x y) yes no)
(If (Neq64 x y) yes no) -> (BRANCH (BNE x y) yes no)
(If (Less64 x y) yes no) -> (BRANCH (BLT x y) yes no)
(If (Less64U x y) yes no) -> (BRANCH (BLTU x y) yes no)
(If (Geq64 x y) yes no) -> (BRANCH (BGE x y) yes no)
(If (Geq64U x y) yes no) -> (BRANCH (BGEU x y) yes no)

// Conditional branches using reversed operands
(If (Leq64 x y) yes no) -> (BRANCH (BGE y x) yes no)
(If (Leq64U x y) yes no) -> (BRANCH (BGEU y x) yes no)
(If (Greater64 x y) yes no) -> (BRANCH (BLT y x) yes no)
(If (Greater64U x y) yes no) -> (BRANCH (BLTU y x) yes no)

// Conditional branches on a boolean value
// This must be last in the rules file, so that all specialized If block rewrites take higher priority.
(If cond yes no) && cond.Type.IsBoolean() -> (BRANCH (BNE (MOVDconst <cond.Type>) (ANDI <cond.Type> [1] cond)) yes no)
//
// cond is 1 if true. BNE compares against 0.
//
// TODO(prattmic): RISCV branch instructions take two operands to compare,
// so we could generate more efficient code by computing the condition in the
// branch itself. Unfortunately, the compiler doesn't current support Blocks
// with two control values. Revisit adding support for two control values.
(If cond yes no) -> (BNE cond yes no)

// Calls
(StaticCall [argwid] {target} mem) -> (CALLstatic [argwid] {target} mem)
Expand Down
19 changes: 1 addition & 18 deletions src/cmd/compile/internal/ssa/gen/RISCVOps.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,8 @@ func init() {
var (
gpstore = regInfo{inputs: []regMask{gpspsbMask, gpspMask, 0}} // SB in first input so we can load from a global, but not in second to avoid using SB as a temporary register
gp01 = regInfo{outputs: []regMask{gpMask}}
// FIXME(prattmic): This is a hack to get things to build, but it probably
// not correct.
gp11 = regInfo{inputs: []regMask{gpMask}, outputs: []regMask{gpMask}}
gp21 = regInfo{inputs: []regMask{gpMask, gpMask}, outputs: []regMask{gpMask}}
gp20 = regInfo{inputs: []regMask{gpMask, gpMask}, outputs: []regMask{}}
gpload = regInfo{inputs: []regMask{gpspsbMask, 0}, outputs: []regMask{gpMask}}
gp11sb = regInfo{inputs: []regMask{gpspsbMask}, outputs: []regMask{gpMask}}

Expand Down Expand Up @@ -161,20 +158,6 @@ func init() {
{name: "SLTU", argLength: 2, reg: gp21, asm: "SLTU"}, // arg0 < arg1, unsigned, result is 0 or 1
{name: "SLTIU", argLength: 1, reg: gp11, asm: "SLTIU", aux: "Int64"}, // arg0 < auxint, unsigned, result is 0 or 1

// Flag pseudo-ops.
// RISC-V doesn't have flags, but SSA wants branches to be flag-based.
// These are pseudo-ops that contain the arguments to the comparison.
// There is a single branching block type, BRANCH,
// which accepts one of these values as its control.
// During code generation we consult the control value
// to emit the correct conditional branch instruction.
{name: "BEQ", argLength: 2, reg: gp20, asm: "BEQ", typ: "Flags"}, // branch if arg0 == arg1
{name: "BNE", argLength: 2, reg: gp20, asm: "BNE", typ: "Flags"}, // branch if arg0 != arg1
{name: "BLT", argLength: 2, reg: gp20, asm: "BLT", typ: "Flags"}, // branch if arg0 < arg1
{name: "BLTU", argLength: 2, reg: gp20, asm: "BLTU", typ: "Flags"}, // branch if arg0 < arg1, unsigned
{name: "BGE", argLength: 2, reg: gp20, asm: "BGE", typ: "Flags"}, // branch if arg0 >= arg1
{name: "BGEU", argLength: 2, reg: gp20, asm: "BGEU", typ: "Flags"}, // branch if arg0 >= arg1, unsigned

// MOVconvert converts between pointers and integers.
// We have a special op for this so as to not confuse GC
// (particularly stack maps). It takes a memory arg so it
Expand Down Expand Up @@ -284,7 +267,7 @@ func init() {
}

RISCVblocks := []blockData{
{name: "BRANCH"}, // see flag pseudo-ops above.
{name: "BNE"}, // Control != 0 (take a register)
}

archs = append(archs, arch{
Expand Down
76 changes: 2 additions & 74 deletions src/cmd/compile/internal/ssa/opGen.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ const (
BlockPPC64FGT
BlockPPC64FGE

BlockRISCVBRANCH
BlockRISCVBNE

BlockS390XEQ
BlockS390XNE
Expand Down Expand Up @@ -192,7 +192,7 @@ var blockString = [...]string{
BlockPPC64FGT: "FGT",
BlockPPC64FGE: "FGE",

BlockRISCVBRANCH: "BRANCH",
BlockRISCVBNE: "BNE",

BlockS390XEQ: "EQ",
BlockS390XNE: "NE",
Expand Down Expand Up @@ -1296,12 +1296,6 @@ const (
OpRISCVSLTI
OpRISCVSLTU
OpRISCVSLTIU
OpRISCVBEQ
OpRISCVBNE
OpRISCVBLT
OpRISCVBLTU
OpRISCVBGE
OpRISCVBGEU
OpRISCVMOVconvert
OpRISCVCALLstatic
OpRISCVCALLclosure
Expand Down Expand Up @@ -16235,72 +16229,6 @@ var opcodeTable = [...]opInfo{
},
},
},
{
name: "BEQ",
argLen: 2,
asm: riscv.ABEQ,
reg: regInfo{
inputs: []inputInfo{
{0, 1073741812}, // GP T0 T1 T2 S0 S1 A0 A1 A2 A3 A4 A5 A6 A7 S2 S3 CTXT S5 S6 S7 S8 S9 S10 S11 T3 T4 T5
{1, 1073741812}, // GP T0 T1 T2 S0 S1 A0 A1 A2 A3 A4 A5 A6 A7 S2 S3 CTXT S5 S6 S7 S8 S9 S10 S11 T3 T4 T5
},
},
},
{
name: "BNE",
argLen: 2,
asm: riscv.ABNE,
reg: regInfo{
inputs: []inputInfo{
{0, 1073741812}, // GP T0 T1 T2 S0 S1 A0 A1 A2 A3 A4 A5 A6 A7 S2 S3 CTXT S5 S6 S7 S8 S9 S10 S11 T3 T4 T5
{1, 1073741812}, // GP T0 T1 T2 S0 S1 A0 A1 A2 A3 A4 A5 A6 A7 S2 S3 CTXT S5 S6 S7 S8 S9 S10 S11 T3 T4 T5
},
},
},
{
name: "BLT",
argLen: 2,
asm: riscv.ABLT,
reg: regInfo{
inputs: []inputInfo{
{0, 1073741812}, // GP T0 T1 T2 S0 S1 A0 A1 A2 A3 A4 A5 A6 A7 S2 S3 CTXT S5 S6 S7 S8 S9 S10 S11 T3 T4 T5
{1, 1073741812}, // GP T0 T1 T2 S0 S1 A0 A1 A2 A3 A4 A5 A6 A7 S2 S3 CTXT S5 S6 S7 S8 S9 S10 S11 T3 T4 T5
},
},
},
{
name: "BLTU",
argLen: 2,
asm: riscv.ABLTU,
reg: regInfo{
inputs: []inputInfo{
{0, 1073741812}, // GP T0 T1 T2 S0 S1 A0 A1 A2 A3 A4 A5 A6 A7 S2 S3 CTXT S5 S6 S7 S8 S9 S10 S11 T3 T4 T5
{1, 1073741812}, // GP T0 T1 T2 S0 S1 A0 A1 A2 A3 A4 A5 A6 A7 S2 S3 CTXT S5 S6 S7 S8 S9 S10 S11 T3 T4 T5
},
},
},
{
name: "BGE",
argLen: 2,
asm: riscv.ABGE,
reg: regInfo{
inputs: []inputInfo{
{0, 1073741812}, // GP T0 T1 T2 S0 S1 A0 A1 A2 A3 A4 A5 A6 A7 S2 S3 CTXT S5 S6 S7 S8 S9 S10 S11 T3 T4 T5
{1, 1073741812}, // GP T0 T1 T2 S0 S1 A0 A1 A2 A3 A4 A5 A6 A7 S2 S3 CTXT S5 S6 S7 S8 S9 S10 S11 T3 T4 T5
},
},
},
{
name: "BGEU",
argLen: 2,
asm: riscv.ABGEU,
reg: regInfo{
inputs: []inputInfo{
{0, 1073741812}, // GP T0 T1 T2 S0 S1 A0 A1 A2 A3 A4 A5 A6 A7 S2 S3 CTXT S5 S6 S7 S8 S9 S10 S11 T3 T4 T5
{1, 1073741812}, // GP T0 T1 T2 S0 S1 A0 A1 A2 A3 A4 A5 A6 A7 S2 S3 CTXT S5 S6 S7 S8 S9 S10 S11 T3 T4 T5
},
},
},
{
name: "MOVconvert",
argLen: 2,
Expand Down
Loading

0 comments on commit 8abd4ac

Please sign in to comment.