Fix incorrect codegen when two byte locals are reassigned from runtime intermediates (#539)

* Initial plan

* Fix incorrect codegen for dual-assignment from runtime intermediates

Agent-Logs-Url: https://github.com/jonathanpeppers/dotnes/sessions/41175c46-2f90-4f80-9967-605c064e987b

Co-authored-by: jonathanpeppers <840039+jonathanpeppers@users.noreply.github.com>

* Address review: tighten test assertions, drop misleading test

- Anchor TEMP-reload assertions to the specific STA/LDA/STA sequence
  around the two byte local stores so we don't false-match on STA/LDA
  \ emitted by built-in subroutines.
- Drop TwoLocals_UshortIntermediates_DualAssignBack_ReversedOrder: in
  the reversed source order Roslyn doesn't elide the intermediate
  stloc, so the recovery emit never fires. The original assertion was
  passing only because of A5 17 inside NTADR_A built-in code, making
  it a false-positive regression test.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: jonathanpeppers <840039+jonathanpeppers@users.noreply.github.com>
Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: 3 people

src/dotnes.tasks/Utilities/IL2NESWriter.LocalVariables.cs

@@ -76,6 +76,40 @@ void WriteStloc(Local local, bool isNewAllocation = true)
         _firstAndAfterPadPoll = false;
         _ushortInAX = false;
 
+        // Detect the "two pending IL stack values" pattern produced when
+        // Roslyn keeps the second computed value on the IL stack across a
+        // stloc, e.g.:
+        //   x1 = (byte)nx;
+        //   y1 = (byte)ny;   // ny was computed earlier, never stored
+        // becomes IL:
+        //   ldloc nx; conv.u1; stloc x1; conv.u1; stloc y1
+        // with ny already on the evaluation stack BELOW nx. WriteLdloc saves
+        // the runtime ny to TEMP ($17) before loading nx into A, but the
+        // subsequent stloc would otherwise reuse A (still nx) for both stores.
+        // When we see the buffer pattern [STA $TEMP, LDA absolute, STA absolute],
+        // this stloc must consume the saved TEMP value instead of A.
+        if (!local.IsWord && local.Value <= byte.MaxValue
+            && local.LabelName is null && CurrentBlock is { } block
+            && block.Count >= 3)
+        {
+            var last = block[block.Count - 1];
+            var prev = block[block.Count - 2];
+            var prev2 = block[block.Count - 3];
+            if (last.Opcode == Opcode.STA && last.Mode == AddressMode.Absolute
+                && prev.Opcode == Opcode.LDA && prev.Mode == AddressMode.Absolute
+                && prev2.Opcode == Opcode.STA && prev2.Mode == AddressMode.ZeroPage
+                && prev2.Operand is ImmediateOperand tempOp && tempOp.Value == (byte)NESConstants.TEMP)
+            {
+                if (isNewAllocation) LocalCount += 1;
+                Emit(Opcode.LDA, AddressMode.ZeroPage, (byte)NESConstants.TEMP);
+                Emit(Opcode.STA, AddressMode.Absolute, (ushort)local.Address);
+                _runtimeValueInA = false;
+                _savedRuntimeToTemp = false;
+                _immediateInA = null;
+                return;
+            }
+        }
+
         if (_ntadrRuntimeResult)
         {
             // NTADR result is in TEMP ($17 = hi) and TEMP2 ($19 = lo)

src/dotnes.tests/LocalsTests.cs

@@ -529,4 +529,86 @@ public void TwoLocals_AddModulo_AssignBack()
         string between = hex.Substring(tempIdx, temp2Idx - tempIdx);
         Assert.Contains("20", between);
     }
+
+    [Fact]
+    public void TwoLocals_UshortIntermediates_DualAssignBack()
+    {
+        // Regression test from issue #516:
+        // ushort intermediates nx/ny, both byte locals reassigned in sequence.
+        // Roslyn optimizes away `stloc ny` and keeps ny on the IL evaluation
+        // stack across the next `stloc x1`. The transpiler must recover ny
+        // from TEMP for the subsequent `stloc y1`, otherwise y1 gets the
+        // value of nx (producing position (2,2) instead of (2,3)).
+        var bytes = GetProgramBytes("""
+            byte y1 = 1;
+            byte x1 = 1;
+            ushort nx;
+            ushort ny;
+            nx = (byte)((x1 + y1) % 32);
+            ny = (byte)((x1 + y1 * 2) % 16);
+            x1 = (byte)nx;
+            y1 = (byte)ny;
+            vrambuf_put(NTADR_A(x1, y1), "B");
+            while (true) ;
+            """);
+        var hex = Convert.ToHexString(bytes);
+
+        // ny computation must save its runtime result to TEMP ($17): 8517
+        Assert.Contains("8517", hex);
+
+        // After storing x1=nx (STA $0326 = 8D2603), the next stloc must
+        // LDA TEMP (A517) to recover ny before STA y1 (8D2503).
+        // This is the exact sequence that the fix introduces.
+        Assert.Contains("8D2603A5178D2503", hex);
+    }
+
+    [Fact]
+    public void TwoByteLocals_RuntimeIntermediates_DualAssignBack()
+    {
+        // Same pattern but using byte intermediates instead of ushort. The
+        // fix should generalize: when Roslyn keeps a runtime value on the
+        // IL stack across an intervening stloc, the next stloc must recover
+        // it from TEMP.
+        var bytes = GetProgramBytes("""
+            byte y1 = 1;
+            byte x1 = 1;
+            byte nx;
+            byte ny;
+            nx = (byte)((x1 + y1) % 32);
+            ny = (byte)((x1 + y1 * 2) % 16);
+            x1 = nx;
+            y1 = ny;
+            vrambuf_put(NTADR_A(x1, y1), "B");
+            while (true) ;
+            """);
+        var hex = Convert.ToHexString(bytes);
+        // Anchored recovery sequence: STA $0326 (x1=nx) → LDA $17 (recover ny)
+        // → STA $0325 (y1=ny). Matching the full sequence avoids false
+        // positives from STA/LDA $17 inside built-in subroutines.
+        Assert.Contains("8D2603A5178D2503", hex);
+    }
+
+    [Fact]
+    public void TwoLocals_UshortIntermediates_NoNTADR()
+    {
+        // Same pattern without NTADR_A — verify the fix is independent of
+        // the call site. After the dual assignment, x1+y1 should be 5
+        // (nx=2, ny=3 → x1=2, y1=3 → 2+3=5), so pal_col(2, 5) is emitted.
+        var bytes = GetProgramBytes("""
+            byte y1 = 1;
+            byte x1 = 1;
+            ushort nx;
+            ushort ny;
+            nx = (byte)((x1 + y1) % 32);
+            ny = (byte)((x1 + y1 * 2) % 16);
+            x1 = (byte)nx;
+            y1 = (byte)ny;
+            pal_col(x1, y1);
+            while (true) ;
+            """);
+        var hex = Convert.ToHexString(bytes);
+        // Anchored recovery sequence around the two byte local stores,
+        // independent of the call site (pal_col instead of NTADR_A).
+        Assert.Contains("8D2603A5178D2503", hex);
+    }
 }