tests/fixedbugs: regression test for issue 100 (#129)

Adds a regression test based on klauspost/compress#186. This necessitated some related changes:

* Mark "RET" as a terminal instruction
* printer refactor to maintain compatibility with asmfmt
* Tweaks to other regression tests to ensure they are run correctly in CI

Updates #100 #65 #8

examples/stadtx/stadtx.s

@@ -33,10 +33,10 @@ TEXT ·Hash(SB), NOSPLIT, $0-40
 	JE    shortCore3
 
 shortCore3:
-	MOVQ  (CX), SI
+	MOVQ  (CX), AX
-	MOVQ  $0x9c1b8e1e9628323f, DI
+	MOVQ  $0x9c1b8e1e9628323f, SI
-	IMULQ DI, SI
+	IMULQ SI, AX
-	ADDQ  SI, BX
+	ADDQ  AX, BX
 	RORQ  $0x11, BX
 	XORQ  BP, BX
 	RORQ  $0x35, BP
@@ -45,10 +45,10 @@ shortCore3:
 	SUBQ  $0x00000008, DX
 
 shortCore2:
-	MOVQ  (CX), SI
+	MOVQ  (CX), AX
-	MOVQ  $0x9c1b8e1e9628323f, DI
+	MOVQ  $0x9c1b8e1e9628323f, SI
-	IMULQ DI, SI
+	IMULQ SI, AX
-	ADDQ  SI, BX
+	ADDQ  AX, BX
 	RORQ  $0x11, BX
 	XORQ  BP, BX
 	RORQ  $0x35, BP
@@ -57,10 +57,10 @@ shortCore2:
 	SUBQ  $0x00000008, DX
 
 shortCore1:
-	MOVQ  (CX), SI
+	MOVQ  (CX), AX
-	MOVQ  $0x9c1b8e1e9628323f, DI
+	MOVQ  $0x9c1b8e1e9628323f, SI
-	IMULQ DI, SI
+	IMULQ SI, AX
-	ADDQ  SI, BX
+	ADDQ  AX, BX
 	RORQ  $0x11, BX
 	XORQ  BP, BX
 	RORQ  $0x35, BP
@@ -87,38 +87,38 @@ shortCore0:
 	JE   shortTail7
 
 shortTail7:
-	MOVBQZX 6(CX), SI
+	MOVBQZX 6(CX), DX
-	SHLQ    $0x20, SI
+	SHLQ    $0x20, DX
-	ADDQ    SI, BX
+	ADDQ    DX, BX
 
 shortTail6:
-	MOVBQZX 5(CX), SI
+	MOVBQZX 5(CX), DX
-	SHLQ    $0x30, SI
+	SHLQ    $0x30, DX
-	ADDQ    SI, BP
+	ADDQ    DX, BP
 
 shortTail5:
-	MOVBQZX 4(CX), SI
+	MOVBQZX 4(CX), DX
-	SHLQ    $0x10, SI
+	SHLQ    $0x10, DX
-	ADDQ    SI, BX
+	ADDQ    DX, BX
 
 shortTail4:
-	MOVLQZX (CX), SI
+	MOVLQZX (CX), DX
-	ADDQ    SI, BP
+	ADDQ    DX, BP
 	JMP     shortAfter
 
 shortTail3:
-	MOVBQZX 2(CX), SI
+	MOVBQZX 2(CX), DX
-	SHLQ    $0x30, SI
+	SHLQ    $0x30, DX
-	ADDQ    SI, BX
+	ADDQ    DX, BX
 
 shortTail2:
-	MOVWQZX (CX), SI
+	MOVWQZX (CX), DX
-	ADDQ    SI, BP
+	ADDQ    DX, BP
 	JMP     shortAfter
 
 shortTail1:
-	MOVBQZX (CX), SI
+	MOVBQZX (CX), DX
-	ADDQ    SI, BX
+	ADDQ    DX, BX
 
 shortTail0:
 	RORQ $0x20, BP
@@ -262,37 +262,37 @@ longCore0:
 	JE    longTail7
 
 longTail7:
-	MOVBQZX 6(CX), SI
+	MOVBQZX 6(CX), DX
-	ADDQ    SI, BP
+	ADDQ    DX, BP
 
 longTail6:
-	MOVWQZX 4(CX), SI
+	MOVWQZX 4(CX), DX
-	ADDQ    SI, DI
+	ADDQ    DX, DI
-	MOVLQZX (CX), SI
+	MOVLQZX (CX), DX
-	ADDQ    SI, AX
+	ADDQ    DX, AX
 	JMP     longAfter
 
 longTail5:
-	MOVBQZX 4(CX), SI
+	MOVBQZX 4(CX), DX
-	ADDQ    SI, BP
+	ADDQ    DX, BP
 
 longTail4:
-	MOVLQZX (CX), SI
+	MOVLQZX (CX), DX
-	ADDQ    SI, DI
+	ADDQ    DX, DI
 	JMP     longAfter
 
 longTail3:
-	MOVBQZX 2(CX), SI
+	MOVBQZX 2(CX), DX
-	ADDQ    SI, AX
+	ADDQ    DX, AX
 
 longTail2:
-	MOVWQZX (CX), SI
+	MOVWQZX (CX), DX
-	ADDQ    SI, BP
+	ADDQ    DX, BP
 	JMP     longAfter
 
 longTail1:
-	MOVBQZX (CX), SI
+	MOVBQZX (CX), DX
-	ADDQ    SI, DI
+	ADDQ    DX, DI
 
 longTail0:
 	ROLQ $0x20, AX

internal/gen/ctors.go

@@ -98,6 +98,10 @@ func construct(i inst.Instruction, f inst.Form, s signature) string {
 	}
 
 	// Branch variables.
+	if i.IsTerminal() {
+		fmt.Fprintf(buf, "\tIsTerminal: true,\n")
+	}
+
 	if i.IsBranch() {
 		fmt.Fprintf(buf, "\tIsBranch: true,\n")
 		fmt.Fprintf(buf, "\tIsConditional: %#v,\n", i.IsConditionalBranch())

ir/ir.go

@@ -61,6 +61,11 @@ type Instruction struct {
 
 func (i *Instruction) node() {}
 
+// IsUnconditionalBranch reports whether i is an unconditional branch.
+func (i Instruction) IsUnconditionalBranch() bool {
+	return i.IsBranch && !i.IsConditional
+}
+
 // TargetLabel returns the label referenced by this instruction. Returns nil if
 // no label is referenced.
 func (i Instruction) TargetLabel() *Label {

pass/cfg.go

@@ -62,7 +62,7 @@ func CFG(fn *ir.Function) error {
 		// Otherwise, could continue to the following instruction.
 		switch {
 		case cur.IsTerminal:
-		case cur.IsBranch && !cur.IsConditional:
+		case cur.IsUnconditionalBranch():
 		default:
 			cur.Succ = append(cur.Succ, nxt)
 		}

printer/goasm.go

@@ -1,10 +1,8 @@
 package printer
 
 import (
-	"fmt"
 	"strconv"
 	"strings"
-	"text/tabwriter"
 
 	"github.com/mmcloughlin/avo/internal/prnt"
 	"github.com/mmcloughlin/avo/ir"
@@ -17,6 +15,9 @@ const dot = "\u00b7"
 type goasm struct {
 	cfg Config
 	prnt.Generator
+
+	instructions []*ir.Instruction
+	clear        bool
 }
 
 // NewGoAsm constructs a printer for writing Go assembly files.
@@ -87,31 +88,21 @@ func (p *goasm) function(f *ir.Function) {
 	}
 	p.Printf(", %s\n", textsize(f))
 
-	w := p.tabwriter()
+	p.clear = true
-	clear := true
-	flush := func() {
-		w.Flush()
-		w = p.tabwriter()
-		if !clear {
-			p.NL()
-			clear = true
-		}
-	}
 	for _, node := range f.Nodes {
 		switch n := node.(type) {
 		case *ir.Instruction:
-			leader := []byte{tabwriter.Escape, '\t', tabwriter.Escape}
+			p.instruction(n)
-			fmt.Fprint(w, string(leader)+n.Opcode)
+			if n.IsTerminal || n.IsUnconditionalBranch() {
-			if len(n.Operands) > 0 {
+				p.flush()
-				fmt.Fprintf(w, "\t%s", joinOperands(n.Operands))
 			}
-			fmt.Fprint(w, "\n")
-			clear = false
 		case ir.Label:
-			flush()
+			p.flush()
+			p.ensureclear()
 			p.Printf("%s:\n", n)
 		case *ir.Comment:
-			flush()
+			p.flush()
+			p.ensureclear()
 			for _, line := range n.Lines {
 				p.Printf("\t// %s\n", line)
 			}
@@ -119,11 +110,45 @@ func (p *goasm) function(f *ir.Function) {
 			panic("unexpected node type")
 		}
 	}
-	w.Flush()
+	p.flush()
 }
 
-func (p *goasm) tabwriter() *tabwriter.Writer {
+func (p *goasm) instruction(i *ir.Instruction) {
-	return tabwriter.NewWriter(p.Raw(), 4, 4, 1, ' ', tabwriter.StripEscape)
+	p.instructions = append(p.instructions, i)
+	p.clear = false
+}
+
+func (p *goasm) flush() {
+	if len(p.instructions) == 0 {
+		return
+	}
+
+	// Determine instruction width. Instructions with no operands are not
+	// considered in this calculation.
+	width := 0
+	for _, i := range p.instructions {
+		if len(i.Operands) > 0 && len(i.Opcode) > width {
+			width = len(i.Opcode)
+		}
+	}
+
+	// Output instruction block.
+	for _, i := range p.instructions {
+		if len(i.Operands) > 0 {
+			p.Printf("\t%-*s%s\n", width+1, i.Opcode, joinOperands(i.Operands))
+		} else {
+			p.Printf("\t%s\n", i.Opcode)
+		}
+	}
+
+	p.instructions = nil
+}
+
+func (p *goasm) ensureclear() {
+	if !p.clear {
+		p.NL()
+		p.clear = true
+	}
 }
 
 func (p *goasm) global(g *ir.Global) {

printer/goasm_test.go

@@ -81,3 +81,27 @@ func TestConstraints(t *testing.T) {
 		"",
 	})
 }
+
+func TestAlignmentNoOperands(t *testing.T) {
+	ctx := build.NewContext()
+	ctx.Function("alignment")
+	ctx.SignatureExpr("func()")
+	ctx.ADDQ(reg.RAX, reg.RBX)
+	ctx.VMOVDQU(reg.Y4, reg.Y11)
+	ctx.VZEROUPPER()
+	ctx.ADDQ(reg.R9, reg.R13)
+	ctx.RET()
+
+	AssertPrintsLines(t, ctx, printer.NewGoAsm, []string{
+		"// Code generated by avo. DO NOT EDIT.",
+		"",
+		"// func alignment()",
+		"TEXT ·alignment(SB), $0",
+		"\tADDQ    AX, BX",
+		"\tVMOVDQU Y4, Y11",
+		"\tVZEROUPPER",      // instruction with no alignment doesn't affect width
+		"\tADDQ    R9, R13", // retains alignment from above
+		"\tRET",
+		"",
+	})
+}

tests/fixedbugs/issue100/allocfail/LICENSE

@@ -0,0 +1,28 @@
+Copyright (c) 2011 The Snappy-Go Authors. All rights reserved.
+Copyright (c) 2019 Klaus Post. All rights reserved.
+
+Redistribution and use in source and binary forms, with or without
+modification, are permitted provided that the following conditions are
+met:
+
+   * Redistributions of source code must retain the above copyright
+notice, this list of conditions and the following disclaimer.
+   * Redistributions in binary form must reproduce the above
+copyright notice, this list of conditions and the following disclaimer
+in the documentation and/or other materials provided with the
+distribution.
+   * Neither the name of Google Inc. nor the names of its
+contributors may be used to endorse or promote products derived from
+this software without specific prior written permission.
+
+THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

tests/fixedbugs/issue100/allocfail/README.md

@@ -0,0 +1,11 @@
+Regression test for [issue
+#100](https://github.com/mmcloughlin/avo/issues/100) based on the original
+reported allocation failure.
+
+Based on the pull request
+[`klauspost/compress#186`](https://github.com/klauspost/compress/pull/186) at
+`c1f3cf132cd8e214b38cc16e418bf2e501ccda93` with the lines after `FIXME`
+comments re-activated and other minimal edits to make it work in this
+environment.
+
+Original code covered by [BSD 3-Clause License](LICENSE).

tests/fixedbugs/issue100/allocfail/doc.go

@@ -0,0 +1,9 @@
+// Package allocfail is a regression test for issue 100 based on the original reported allocation failure.
+//
+// Based on the pull request https://github.com/klauspost/compress/pull/186 at
+// c1f3cf132cd8e214b38cc16e418bf2e501ccda93 with the lines after "FIXME"
+// comments re-activated and other minimal edits to make it work in this
+// environment.
+package allocfail
+
+//go:generate go run asm.go -out allocfail.s -stubs stubs.go

tests/fixedbugs/issue100/allocfail/stubs.go

@@ -0,0 +1,85 @@
+// Code generated by command: go run asm.go -out allocfail.s -stubs stubs.go. DO NOT EDIT.
+
+// +build !appengine
+// +build !noasm
+// +build gc
+
+package allocfail
+
+// encodeBlockAsm encodes a non-empty src to a guaranteed-large-enough dst.
+// It assumes that the varint-encoded length of the decompressed bytes has already been written.
+//
+//go:noescape
+func encodeBlockAsm(dst []byte, src []byte) int
+
+// encodeBlockAsm14B encodes a non-empty src to a guaranteed-large-enough dst.
+// It assumes that the varint-encoded length of the decompressed bytes has already been written.
+//
+//go:noescape
+func encodeBlockAsm14B(dst []byte, src []byte) int
+
+// encodeBlockAsm12B encodes a non-empty src to a guaranteed-large-enough dst.
+// It assumes that the varint-encoded length of the decompressed bytes has already been written.
+//
+//go:noescape
+func encodeBlockAsm12B(dst []byte, src []byte) int
+
+// encodeBlockAsmAvx encodes a non-empty src to a guaranteed-large-enough dst.
+// It assumes that the varint-encoded length of the decompressed bytes has already been written.
+//
+//go:noescape
+func encodeBlockAsmAvx(dst []byte, src []byte) int
+
+// encodeBlockAsm14BAvx encodes a non-empty src to a guaranteed-large-enough dst.
+// It assumes that the varint-encoded length of the decompressed bytes has already been written.
+//
+//go:noescape
+func encodeBlockAsm14BAvx(dst []byte, src []byte) int
+
+// encodeBlockAsm12BAvx encodes a non-empty src to a guaranteed-large-enough dst.
+// It assumes that the varint-encoded length of the decompressed bytes has already been written.
+//
+//go:noescape
+func encodeBlockAsm12BAvx(dst []byte, src []byte) int
+
+// emitLiteral writes a literal chunk and returns the number of bytes written.
+//
+// It assumes that:
+//   dst is long enough to hold the encoded bytes
+//   0 <= len(lit) && len(lit) <= math.MaxUint32
+//
+//go:noescape
+func emitLiteral(dst []byte, lit []byte) int
+
+// emitLiteralAvx writes a literal chunk and returns the number of bytes written.
+//
+// It assumes that:
+//   dst is long enough to hold the encoded bytes
+//   0 <= len(lit) && len(lit) <= math.MaxUint32
+//
+//go:noescape
+func emitLiteralAvx(dst []byte, lit []byte) int
+
+// emitRepeat writes a repeat chunk and returns the number of bytes written.
+// Length must be at least 4 and < 1<<32
+//
+//go:noescape
+func emitRepeat(dst []byte, offset int, length int) int
+
+// emitCopy writes a copy chunk and returns the number of bytes written.
+//
+// It assumes that:
+//   dst is long enough to hold the encoded bytes
+//   1 <= offset && offset <= math.MaxUint32
+//   4 <= length && length <= 1 << 24
+//
+//go:noescape
+func emitCopy(dst []byte, offset int, length int) int
+
+// matchLen returns how many bytes match in a and b
+//
+// It assumes that:
+//   len(a) <= len(b)
+//
+//go:noescape
+func matchLen(a []byte, b []byte) int

tests/fixedbugs/issue100/doc.go

@@ -1,2 +0,0 @@
-// Package issue100 contains a reproducer for a bug in aliased register allocation.
-package issue100

tests/fixedbugs/issue100/minrepro/doc.go

@@ -0,0 +1,2 @@
+// Package minrepro contains a minimal reproducer for the aliased register allocation bug in issue 100.
+package minrepro

tests/fixedbugs/issue100/minrepro/minrepro.s

@@ -1,4 +1,4 @@
-// Code generated by command: go run asm.go -out issue100.s -stubs stub.go. DO NOT EDIT.
+// Code generated by command: go run asm.go -out minrepro.s -stubs stub.go. DO NOT EDIT.
 
 #include "textflag.h"
 

tests/fixedbugs/issue100/minrepro/minrepro_test.go

@@ -1,10 +1,10 @@
-package issue100
+package minrepro
 
 import (
 	"testing"
 )
 
-//go:generate go run asm.go -out issue100.s -stubs stub.go
+//go:generate go run asm.go -out minrepro.s -stubs stub.go
 
 func TestIssue100(t *testing.T) {
 	n := uint64(100)

tests/fixedbugs/issue100/minrepro/stub.go

@@ -0,0 +1,5 @@
+// Code generated by command: go run asm.go -out minrepro.s -stubs stub.go. DO NOT EDIT.
+
+package minrepro
+
+func Issue100() uint64

tests/fixedbugs/issue100/stub.go

@@ -1,5 +0,0 @@
-// Code generated by command: go run asm.go -out issue100.s -stubs stub.go. DO NOT EDIT.
-
-package issue100
-
-func Issue100() uint64

tests/fixedbugs/issue65/asm.go

@@ -1,11 +1,4 @@
-// +build generate
+// +build ignore
-
-//go:generate go run $GOFILE
-
-// Regression test for a bug where casting a physical register would give the
-// error "non physical register found".
-//
-// See: https://github.com/mmcloughlin/avo/issues/65#issuecomment-576850145
 
 package main
 

tests/fixedbugs/issue65/doc.go

@@ -0,0 +1,9 @@
+// Package issue65 is a regression test for a bug involving casting physical registers.
+//
+// Regression test for a bug where casting a physical register would give the
+// error "non physical register found".
+//
+// See: https://github.com/mmcloughlin/avo/issues/65#issuecomment-576850145
+package issue65
+
+//go:generate go run asm.go -out issue65.s -stubs stub.go

tests/fixedbugs/issue65/issue65.s

@@ -0,0 +1,9 @@
+// Code generated by command: go run asm.go -out issue65.s -stubs stub.go. DO NOT EDIT.
+
+#include "textflag.h"
+
+// func Issue65()
+// Requires: AVX2
+TEXT ·Issue65(SB), NOSPLIT, $0
+	VINSERTI128 $0x01, X0, Y1, Y2
+	RET

tests/fixedbugs/issue65/stub.go

@@ -0,0 +1,5 @@
+// Code generated by command: go run asm.go -out issue65.s -stubs stub.go. DO NOT EDIT.
+
+package issue65
+
+func Issue65()

x86/zctors.go

@@ -15578,10 +15578,11 @@ func RDTSCP() (*intrep.Instruction, error) {
 // 	RET
 func RET() (*intrep.Instruction, error) {
 	return &intrep.Instruction{
-		Opcode:   "RET",
+		Opcode:     "RET",
-		Operands: nil,
+		Operands:   nil,
-		Inputs:   []operand.Op{},
+		Inputs:     []operand.Op{},
-		Outputs:  []operand.Op{},
+		Outputs:    []operand.Op{},
+		IsTerminal: true,
 	}, nil
 }