From bc0787fefb637395c17c90558f9d22d6fcd89281 Mon Sep 17 00:00:00 2001
From: Joe Savona <joesavona@fb.com>
Date: Tue, 20 Dec 2022 16:09:59 -0800
Subject: [PATCH] More precise handling of const/reassign in ssa form

Addressed a TODO from the previous PR. When we enter SSA form, when we rewrite
variable reassignments we currently change the identifier but leave the kind of
the lvalue alone; technically we should convert from Reassign to Const. After
doing that, it's easier to correctly update when we leave SSA form, we can
convert just a subset back into let/reassign (but leave most things alone as
const).
---
 compiler/forget/src/SSA/EnterSSA.ts |  2 ++
 compiler/forget/src/SSA/LeaveSSA.ts | 23 +++++++++++------------
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/compiler/forget/src/SSA/EnterSSA.ts b/compiler/forget/src/SSA/EnterSSA.ts
index 4c710bc282..eb2a4db3e0 100644
--- a/compiler/forget/src/SSA/EnterSSA.ts
+++ b/compiler/forget/src/SSA/EnterSSA.ts
@@ -3,6 +3,7 @@ import {
   HIRFunction,
   Identifier,
   IdentifierId,
+  InstructionKind,
   makeInstructionId,
   makeType,
   Phi,
@@ -202,6 +203,7 @@ export default function enterSSA(func: HIRFunction, env: Environment) {
           newPlace = builder.getPlace(oldPlace);
         } else {
           newPlace = builder.definePlace(oldPlace);
+          instr.lvalue.kind = InstructionKind.Const;
         }
         instr.lvalue.place = newPlace;
       }
diff --git a/compiler/forget/src/SSA/LeaveSSA.ts b/compiler/forget/src/SSA/LeaveSSA.ts
index 27ec13dec8..a560a11408 100644
--- a/compiler/forget/src/SSA/LeaveSSA.ts
+++ b/compiler/forget/src/SSA/LeaveSSA.ts
@@ -291,23 +291,22 @@ export function leaveSSA(fn: HIRFunction) {
     // Finally, iterate the instructions and perform any rewrites as well as converting
     // SSA variables to `const` where possible
     for (const instr of block.instructions) {
-      const { lvalue, value } = instr;
+      const { lvalue } = instr;
       if (lvalue !== null) {
-        rewritePlace(lvalue.place, rewrites);
         if (
-          lvalue.kind !== InstructionKind.Const &&
+          lvalue.kind === InstructionKind.Const &&
           lvalue.place.memberPath === null &&
-          !rewrites.has(lvalue.place.identifier) &&
-          (!reassignments.has(lvalue.place.identifier) ||
-            reassignments.get(lvalue.place.identifier) !==
-              lvalue.place.identifier)
+          rewrites.has(lvalue.place.identifier)
         ) {
-          // Convert individual SSA reassignments into const declarations
-          // otherwise the code would be invalid, since the SSA identifiers
-          // aren't otherwise declared.
-          // TODO @josephsavona: do this in EnterSSA instead?
-          lvalue.kind = InstructionKind.Const;
+          // For rewrites, the declaration of the canonical identifier has to be `let`,
+          // all other assignments are reassignments (which we annotate for codegen
+          // purposes).
+          lvalue.kind =
+            rewrites.get(lvalue.place.identifier) === lvalue.place.identifier
+              ? InstructionKind.Let
+              : InstructionKind.Reassign;
         }
+        rewritePlace(lvalue.place, rewrites);
       }
       for (const operand of eachInstructionValueOperand(instr.value)) {
         rewritePlace(operand, rewrites);