Repository: tetratelabs/wazero
Pull request #647 (2022-06-21 06:25:32)

amd64,simd: fix occational partial failure of FP neg. 
This fixes amd64's compileV128NegFloat so that the impl won't be
affected by the previous values on the temporary vector register.

This was a flaky bug found in 
* https://github.com/tetratelabs/wazero/runs/6977169039?check_suite_focus=true
* https://github.com/tetratelabs/wazero/runs/6958490906?check_suite_focus=true




Patch:
diff --git a/internal/engine/compiler/impl_vec_amd64.go b/internal/engine/compiler/impl_vec_amd64.go
index 136a5862..01974b13 100644
--- a/internal/engine/compiler/impl_vec_amd64.go
+++ b/internal/engine/compiler/impl_vec_amd64.go
@@ -1589,9 +1589,14 @@ func (c *amd64Compiler) compileV128NegFloat(s wazeroir.Shape) error {
 		leftShiftInst, leftShiftAmount, xorInst = amd64.PSLLQ, 63, amd64.XORPD
 	}
 
-	// Set all bits on tmp by CMPPD with arg=0 (== pseudo CMPEQPS instruction).
+	// Clear all bits on tmp.
+	c.assembler.CompileRegisterToRegister(amd64.XORPS, tmp, tmp)
+	// Set all bits on tmp by CMPPD with arg=0 (== pseudo CMPEQPD instruction).
 	// See https://www.felixcloutier.com/x86/cmpps
-	c.assembler.CompileRegisterToRegisterWithArg(amd64.CMPPD, tmp, tmp, 0)
+	//
+	// Note: if we do not clear all the bits ^ with XORPS, this might end up not setting ones on some lane
+	// if the lane is NaN.
+	c.assembler.CompileRegisterToRegisterWithArg(amd64.CMPPD, tmp, tmp, 0x8)
 	// Do the left shift on each lane to set only the most significant bit in each.
 	c.assembler.CompileConstToRegister(leftShiftInst, leftShiftAmount, tmp)
 	// Get the negated result by XOR on each lane with tmp.


Test patch:
diff --git a/internal/engine/compiler/impl_vec_amd64_test.go b/internal/engine/compiler/impl_vec_amd64_test.go
index a26283ca..cd9903a7 100644
--- a/internal/engine/compiler/impl_vec_amd64_test.go
+++ b/internal/engine/compiler/impl_vec_amd64_test.go
@@ -2,6 +2,7 @@ package compiler
 
 import (
 	"encoding/binary"
+	"math"
 	"testing"
 
 	"github.com/tetratelabs/wazero/internal/asm/amd64"
@@ -216,3 +217,88 @@ func TestAmd64Compiler_compileV128ShrI64x2SignedImpl(t *testing.T) {
 		})
 	}
 }
+
+// TestAmd64Compiler_compileV128Neg_NaNOnTemporary ensures compileV128Neg for floating point variants works well
+// even if the temporary register used by the instruction holds NaN values previously.
+func TestAmd64Compiler_compileV128Neg_NaNOnTemporary(t *testing.T) {
+	tests := []struct {
+		name   string
+		shape  wazeroir.Shape
+		v, exp [16]byte
+	}{
+		{
+			name:  "f32x4",
+			shape: wazeroir.ShapeF32x4,
+			v:     f32x4(51234.12341, -123, float32(math.Inf(1)), 0.1),
+			exp:   f32x4(-51234.12341, 123, float32(math.Inf(-1)), -0.1),
+		},
+		{
+			name:  "f32x4",
+			shape: wazeroir.ShapeF32x4,
+			v:     f32x4(51234.12341, 0, float32(math.Inf(1)), 0.1),
+			exp:   f32x4(-51234.12341, float32(math.Copysign(0, -1)), float32(math.Inf(-1)), -0.1),
+		},
+		{
+			name:  "f64x2",
+			shape: wazeroir.ShapeF64x2,
+			v:     f64x2(1.123, math.Inf(-1)),
+			exp:   f64x2(-1.123, math.Inf(1)),
+		},
+		{
+			name:  "f64x2",
+			shape: wazeroir.ShapeF64x2,
+			v:     f64x2(0, math.Inf(-1)),
+			exp:   f64x2(math.Copysign(0, -1), math.Inf(1)),
+		},
+	}
+
+	for _, tc := range tests {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			env := newCompilerEnvironment()
+			compiler := env.requireNewCompiler(t, newCompiler,
+				&wazeroir.CompilationResult{HasMemory: true, Signature: &wasm.FunctionType{}})
+
+			err := compiler.compilePreamble()
+			require.NoError(t, err)
+
+			err = compiler.compileV128Const(&wazeroir.OperationV128Const{
+				Lo: binary.LittleEndian.Uint64(tc.v[:8]),
+				Hi: binary.LittleEndian.Uint64(tc.v[8:]),
+			})
+			require.NoError(t, err)
+
+			// Ensures that the previous state of temporary register used by Neg holds
+			// NaN values.
+			err = compiler.compileV128Const(&wazeroir.OperationV128Const{
+				Lo: math.Float64bits(math.NaN()),
+				Hi: math.Float64bits(math.NaN()),
+			})
+			require.NoError(t, err)
+
+			// Mark that the temp register is available for Neg instruction below.
+			loc := compiler.runtimeValueLocationStack().popV128()
+			compiler.runtimeValueLocationStack().markRegisterUnused(loc.register)
+
+			// Now compiling Neg where it uses temporary register holding NaN values at this point.
+			err = compiler.compileV128Neg(&wazeroir.OperationV128Neg{Shape: tc.shape})
+			require.NoError(t, err)
+
+			err = compiler.compileReturnFunction()
+			require.NoError(t, err)
+
+			// Generate and run the code under test.
+			code, _, _, err := compiler.compile()
+			require.NoError(t, err)
+			env.exec(code)
+
+			require.Equal(t, nativeCallStatusCodeReturned, env.callEngine().statusCode)
+
+			lo, hi := env.stackTopAsV128()
+			var actual [16]byte
+			binary.LittleEndian.PutUint64(actual[:8], lo)
+			binary.LittleEndian.PutUint64(actual[8:], hi)
+			require.Equal(t, tc.exp, actual)
+		})
+	}
+}