From 270446ee2145186ba4af6c0934ad58e4dbf91448 Mon Sep 17 00:00:00 2001 From: Alyssa Rosenzweig Date: Wed, 26 Jun 2024 21:59:50 -0400 Subject: [PATCH] nir: fix miscompiles with rules with INT32_MIN 812b3415 added rules for upcasts with comparisons with a variety of types. The float & unsigned rules should be ok, but the signed integer rules are unsound as currently implemented. This can cause end-to-end miscompiles. I originally hit this issue while debugging a large real world OpenCL kernel. I found the bug symptoms changed when disabling loop unrolling, which tipped me off to a compiler bug. I've reduced it to a minimal test case. Imagine my surprise when I find out the NIR my backend ingested was already constant folded to be wrong. In the minimal test case, during optimization we have NIR: 32 %6 = .... 64 %9 = i2i64 %6 64 %44 = load_const (0x0000000000000001) 1 %45 = ilt %9, %44 (0x1) This is a simple check (int64_t)%6 < 1. nir_opt_algebraic turns this into: 32 %6 = ... 64 %9 = i2i64 %6 64 %44 = load_const (0x0000000000000001) 64 %55 = load_const (0x0000000080000000 = 2147483648) 1 %56 = ilt %55 (0x80000000), %44 (0x1) 64 %57 = load_const (0x000000007fffffff = 2147483647) 1 %58 = ilt %57 (0x7fffffff), %44 (0x1) 32 %59 = i2i32 %44 (0x1) 1 %60 = ilt %6, %59 1 %61 = ior %58, %60 1 %62 = iand %56, %61 This pile of math constant-folds to an unconditional "false"! The problem is %56. At first glance, INT32_MIN < 1 is true so %56 should be true. Indeed, it should. But here's the kicker: both constants are 64-bit here, so the ilt operation is a 64-bit comparison -- that left-hand side is INT32_MIN zero-extended to 64-bit for the signed comparison at 64-bit. So in fact, it evaluates to false, causing the whole expression to go false. If we're going to do a 64-bit comparison for %56, then we need to sign-extend the bound. So we'll just adjust the Python and be on our way, right? Unfortunately the issue is deeper. According to the comment in the generated nir_opt_algebraic.c file, the guilty algebraic rule is: ('ilt', ('i2i64', 'a@32'), '#b') => ('iand', ('ilt', -2147483648, 'b'), ('ior', ('ilt', 2147483647, 'b'), ('ilt', 'a', ('i2i32', 'b')))) From a Python perspective? That rule is correct. -2147483648 < 1 is a true statement. Adjusting the Python rule is not the appropriate solution here, since the issue is more fundamental and might affect other rules. The real problem is the translation of that Python replacement tree into C, incorrectly zero-extending -2147483648 into 0x0000000080000000 instead of sign-extending to 0xffffffff80000000. Crawling down the rabbit hole of the generated algebraic file, we see the constant encoded as: { .constant = { { nir_search_value_constant, 64 }, nir_type_int, { -0x80000000 /* -2147483648 */ }, } }, NIR correctly translates the negative constant to a C level negate operation of its absolute value. This maps to the correct sign-extension... ...for all constants except for INT_MIN. Because that constant lacks a ULL suffix, it is a 32-bit integer. And for this integer (only), negating it hits signed integer overflow (UB!) and then we end up with an effective zero-extension when going to 64-bit. This patch fixes the end-to-end miscompile. Signed-off-by: Alyssa Rosenzweig Reviewed-by: Faith Ekstrand Closes: #11402 Cc: mesa-stable Part-of: --- src/compiler/nir/nir_algebraic.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/compiler/nir/nir_algebraic.py b/src/compiler/nir/nir_algebraic.py index be26b60d32e..b7efcf93c91 100644 --- a/src/compiler/nir/nir_algebraic.py +++ b/src/compiler/nir/nir_algebraic.py @@ -255,7 +255,12 @@ class Constant(Value): if isinstance(self.value, (bool)): return 'NIR_TRUE' if self.value else 'NIR_FALSE' if isinstance(self.value, int): - return hex(self.value) + # Explicitly sign-extend negative integers to 64-bit, ensuring correct + # handling of -INT32_MIN which is not representable in 32-bit. + if self.value < 0: + return hex(struct.unpack('Q', struct.pack('q', self.value))[0]) + else: + return hex(self.value) elif isinstance(self.value, float): return hex(struct.unpack('Q', struct.pack('d', self.value))[0]) else: