Reject uop definitions that declare values as 'unused' that are already cached by prior uops

Author: markshannon

Lib/test/test_generated_cases.py

@@ -31,7 +31,7 @@ def skip_if_different_mount_drives():
 with test_tools.imports_under_tool("cases_generator"):
     from analyzer import StackItem
     import parser
-    from stack import Stack
+    from stack import Stack, StackError
     import tier1_generator
     import optimizer_generator
 
@@ -827,6 +827,24 @@ def test_deopt_and_exit(self):
         with self.assertRaises(Exception):
             self.run_cases_test(input, output)
 
+    def test_unused_cached_value(self):
+
+        input = """
+        op(FIRST, (arg1 -- out)) {
+            out = arg1;
+        }
+
+        op(SECOND, (unused -- unused)) {
+        }
+
+        macro(BOTH) = FIRST + SECOND;
+        """
+        output = """
+        """
+        with self.assertRaises(SyntaxError):
+            self.run_cases_test(input, output)
+
+
 class TestGeneratedAbstractCases(unittest.TestCase):
     def setUp(self) -> None:
         super().setUp()

Programs/test_frozenmain.h

@@ -3448,7 +3448,7 @@ dummy_func(
             }
         }
 
-        op(_CHECK_FUNCTION_VERSION, (func_version/2, callable, unused, unused[oparg] -- callable, unused, unused[oparg])) {
+        op(_CHECK_FUNCTION_VERSION, (func_version/2, callable, self_or_null, unused[oparg] -- callable, self_or_null, unused[oparg])) {
             PyObject *callable_o = PyStackRef_AsPyObjectBorrow(callable);
             EXIT_IF(!PyFunction_Check(callable_o));
             PyFunctionObject *func = (PyFunctionObject *)callable_o;
@@ -3479,7 +3479,6 @@ dummy_func(
             assert(PyStackRef_IsNull(null));
             assert(Py_TYPE(callable_o) == &PyMethod_Type);
             self = PyStackRef_FromPyObjectNew(((PyMethodObject *)callable_o)->im_self);
-            stack_pointer[-1 - oparg] = self;  // Patch stack as it is used by _PY_FRAME_GENERAL
             method = PyStackRef_FromPyObjectNew(((PyMethodObject *)callable_o)->im_func);
             assert(PyFunction_Check(PyStackRef_AsPyObjectBorrow(method)));
             PyStackRef_CLOSE(callable);
@@ -3544,16 +3543,12 @@ dummy_func(
             EXIT_IF(Py_TYPE(PyStackRef_AsPyObjectBorrow(callable)) != &PyMethod_Type);
         }
 
-        op(_INIT_CALL_BOUND_METHOD_EXACT_ARGS, (callable, unused, unused[oparg] -- func, self, unused[oparg])) {
+        op(_INIT_CALL_BOUND_METHOD_EXACT_ARGS, (callable, null, unused[oparg] -- func, self, unused[oparg])) {
             PyObject *callable_o = PyStackRef_AsPyObjectBorrow(callable);
             STAT_INC(CALL, hit);
-            stack_pointer[-1 - oparg] = PyStackRef_FromPyObjectNew(((PyMethodObject *)callable_o)->im_self);  // Patch stack as it is used by _INIT_CALL_PY_EXACT_ARGS
-            stack_pointer[-2 - oparg] = PyStackRef_FromPyObjectNew(((PyMethodObject *)callable_o)->im_func);  // This is used by CALL, upon deoptimization
-            self = stack_pointer[-1 - oparg];
-            func = stack_pointer[-2 - oparg];
+            self = PyStackRef_FromPyObjectNew(((PyMethodObject *)callable_o)->im_self);
+            func = PyStackRef_FromPyObjectNew(((PyMethodObject *)callable_o)->im_func);
             PyStackRef_CLOSE(callable);
-            // self may be unused in tier 1, so silence warnings.
-            (void)self;
         }
 
         op(_CHECK_PEP_523, (--)) {
@@ -3568,7 +3563,7 @@ dummy_func(
             EXIT_IF(code->co_argcount != oparg + (!PyStackRef_IsNull(self_or_null)));
         }
 
-        op(_CHECK_STACK_SPACE, (callable, unused, unused[oparg] -- callable, unused, unused[oparg])) {
+        op(_CHECK_STACK_SPACE, (callable, self_or_null, unused[oparg] -- callable, self_or_null, unused[oparg])) {
             PyObject *callable_o = PyStackRef_AsPyObjectBorrow(callable);
             PyFunctionObject *func = (PyFunctionObject *)callable_o;
             PyCodeObject *code = (PyCodeObject *)func->func_code;

Python/bytecodes.c

@@ -103,6 +103,7 @@ class StackItem:
     condition: str | None
     size: str
     peek: bool = False
+    cached: bool = False
 
     def __str__(self) -> str:
         cond = f" if ({self.condition})" if self.condition else ""

Python/executor_cases.c.h

@@ -23,7 +23,7 @@
 from cwriter import CWriter
 from typing import TextIO, Iterator
 from lexer import Token
-from stack import Stack, SizeMismatch
+from stack import Stack, StackError
 
 DEFAULT_OUTPUT = ROOT / "Python/optimizer_cases.c.h"
 DEFAULT_ABSTRACT_INPUT = (ROOT / "Python/optimizer_bytecodes.c").absolute().as_posix()
@@ -141,7 +141,7 @@ def write_uop(
                     out.emit(stack.push(var))
         out.start_line()
         stack.flush(out, cast_type="_Py_UopsSymbol *", extract_bits=True)
-    except SizeMismatch as ex:
+    except StackError as ex:
         raise analysis_error(ex.args[0], uop.body[0])
 
 

Python/generated_cases.c.h

@@ -113,7 +113,7 @@ def clear(self) -> None:
         self.pushed = []
 
 
-class SizeMismatch(Exception):
+class StackError(Exception):
     pass
 
 
@@ -133,21 +133,26 @@ def pop(self, var: StackItem, extract_bits: bool = False) -> str:
         if self.variables:
             popped = self.variables.pop()
             if popped.size != var.size:
-                raise SizeMismatch(
+                raise StackError(
                     f"Size mismatch when popping '{popped.name}' from stack to assign to {var.name}. "
                     f"Expected {var.size} got {popped.size}"
                 )
             if popped.name == var.name:
+                var.cached = popped.cached
                 return ""
             elif popped.name in UNUSED:
                 self.defined.add(var.name)
                 return (
                     f"{var.name} = {indirect}stack_pointer[{self.top_offset.to_c()}];\n"
                 )
             elif var.name in UNUSED:
+                if popped.cached:
+                    raise StackError(f"Value is declared unused, but is already cached by prior operation")
                 return ""
             else:
                 self.defined.add(var.name)
+                assert popped.cached
+                var.cached = True
                 return f"{var.name} = {popped.name};\n"
         self.base_offset.pop(var)
         if var.name in UNUSED:
@@ -177,6 +182,8 @@ def push(self, var: StackItem) -> str:
             return f"{var.name} = &stack_pointer[{c_offset}];\n"
         else:
             self.top_offset.push(var)
+            if var.name not in UNUSED:
+                var.cached = True
             return ""
 
     def flush(self, out: CWriter, cast_type: str = "uintptr_t", extract_bits: bool = False) -> None:

Tools/cases_generator/analyzer.py

@@ -22,7 +22,7 @@
 )
 from cwriter import CWriter
 from typing import TextIO
-from stack import Stack, SizeMismatch
+from stack import Stack, StackError
 
 
 DEFAULT_OUTPUT = ROOT / "Python/generated_cases.c.h"
@@ -100,8 +100,8 @@ def write_uop(
             out.emit("}\n")
         # out.emit(stack.as_comment() + "\n")
         return offset
-    except SizeMismatch as ex:
-        raise analysis_error(ex.args[0], uop.body[0])
+    except StackError as ex:
+        raise analysis_error(ex.args[0], uop.body[0]) from None
 
 
 def uses_this(inst: Instruction) -> bool:

Tools/cases_generator/optimizer_generator.py

@@ -24,7 +24,7 @@
 from cwriter import CWriter
 from typing import TextIO, Iterator
 from lexer import Token
-from stack import Stack, SizeMismatch
+from stack import Stack, StackError
 
 DEFAULT_OUTPUT = ROOT / "Python/executor_cases.c.h"
 
@@ -176,8 +176,8 @@ def write_uop(uop: Uop, out: CWriter, stack: Stack) -> None:
         if uop.properties.stores_sp:
             for i, var in enumerate(uop.stack.outputs):
                 out.emit(stack.push(var))
-    except SizeMismatch as ex:
-        raise analysis_error(ex.args[0], uop.body[0])
+    except StackError as ex:
+        raise analysis_error(ex.args[0], uop.body[0]) from None
 
 
 SKIPS = ("_EXTENDED_ARG",)