From a6b422da2eb334f25d37e8b3436df89272f0fb20 Mon Sep 17 00:00:00 2001
From: zerbina <100542850+zerbina@users.noreply.github.com>
Date: Wed, 19 Jul 2023 16:55:23 +0200
Subject: [PATCH] cgen: replace `TNodeTable` usage with `Table` (#805)

## Summary

Replace the usage of `TNodeTable` with `Table` in `cgen`. This is a
preparation for moving away from `TNode` in the code generators.

In addition, the change allowed for fixing an issue with the
structural comparison logic that led to `-0.0` and `0.0` being treated
as equal and thus not-equal array construction expressions (e.g.
`[-0.0, 0.0]` and `[0.0, 0.0]`) collapsed into the the same constant
when using the C backend.

## Details

The implementation of `TNodeTable` is almost exactly the same as that of
`Table`, with the largest difference being that `TNodeTable` uses the
`key` and not its stored hash for testing whether a slot is empty. To
be able to use a `PNode` as the key, a distinct type (`ConstrTree`; for
"construction tree") is created that has a structural hash and
structural equality operator.

Both operators are based on the previously used `hashTree` and
`treesEquivalent` procedures from the `treetab` module, with adjustments
made for the specific use case:
- `nkIdent` nodes don't reach the code generator (and thus don't need to
  be considered)
- nodes of integer kind always have their value hashed, as opposed to
  the value being used as the hash (which means a hash is also computed
  for >= 2^32 values when using a 32-bit compiler)
- the bit pattern of floats are compared, not the values themselves

The last change fixes the aforementioned issue with two non-bit-pattern-
equal values resulting from constant array construction expressions
using into the same constant.

---------

Co-authored-by: Saem Ghani <saemghani+github@gmail.com>
---
 compiler/backend/ccgexprs.nim                 |  8 +-
 compiler/backend/ccgliterals.nim              |  4 +-
 compiler/backend/cgen.nim                     |  3 +-
 compiler/backend/cgendata.nim                 | 82 ++++++++++++++++++-
 .../tlifted_constants_with_float.nim          | 47 +++++++++++
 5 files changed, 133 insertions(+), 11 deletions(-)
 create mode 100644 tests/lang_exprs/tlifted_constants_with_float.nim

diff --git a/compiler/backend/ccgexprs.nim b/compiler/backend/ccgexprs.nim
index 1acf76957f5..ea7eb4f14e0 100644
--- a/compiler/backend/ccgexprs.nim
+++ b/compiler/backend/ccgexprs.nim
@@ -65,7 +65,7 @@ proc genLiteral(p: BProc, n: PNode, ty: PType): Rope =
   of nkNilLit:
     let k = if ty == nil: tyPointer else: skipTypes(ty, abstractVarRange).kind
     if k == tyProc and skipTypes(ty, abstractVarRange).callConv == ccClosure:
-      let id = nodeTableTestOrSet(p.module.dataCache, n, p.module.labels)
+      let id = getOrPut(p.module.dataCache, n, p.module.labels)
       result = p.module.tmpBase & rope(id)
       if id == p.module.labels:
         # not found in cache:
@@ -132,7 +132,7 @@ proc genSetNode(p: BProc, n: PNode): Rope =
   var size = int(getSize(p.config, n.typ))
   let cs = toBitSet(p.config, n)
   if size > 8:
-    let id = nodeTableTestOrSet(p.module.dataCache, n, p.module.labels)
+    let id = getOrPut(p.module.dataCache, n, p.module.labels)
     result = p.module.tmpBase & rope(id)
     if id == p.module.labels:
       # not found in cache:
@@ -998,7 +998,7 @@ proc genNewSeqOfCap(p: BProc; e: PNode; d: var TLoc) =
 proc rawConstExpr(p: BProc, n: PNode; d: var TLoc) =
   let t = n.typ
   discard getTypeDesc(p.module, t) # so that any fields are initialized
-  let id = nodeTableTestOrSet(p.module.dataCache, n, p.module.labels)
+  let id = getOrPut(p.module.dataCache, n, p.module.labels)
   fillLoc(d, locData, n, p.module.tmpBase & rope(id), OnStatic)
   if id == p.module.labels:
     # expression not found in the cache:
@@ -2113,7 +2113,7 @@ proc downConv(p: BProc, n: PNode, d: var TLoc) =
 proc exprComplexConst(p: BProc, n: PNode, d: var TLoc) =
   let t = n.typ
   discard getTypeDesc(p.module, t) # so that any fields are initialized
-  let id = nodeTableTestOrSet(p.module.dataCache, n, p.module.labels)
+  let id = getOrPut(p.module.dataCache, n, p.module.labels)
   let tmp = p.module.tmpBase & rope(id)
 
   if id == p.module.labels:
diff --git a/compiler/backend/ccgliterals.nim b/compiler/backend/ccgliterals.nim
index 7980a649d1a..4a0871f1bf2 100644
--- a/compiler/backend/ccgliterals.nim
+++ b/compiler/backend/ccgliterals.nim
@@ -25,7 +25,7 @@ proc genStringLiteralDataOnlyV2(m: BModule, s: string; result: Rope; isConst: bo
        rope(if isConst: "const" else: "")])
 
 proc genStringLiteralV2(m: BModule; n: PNode; isConst: bool): Rope =
-  let id = nodeTableTestOrSet(m.dataCache, n, m.labels)
+  let id = getOrPut(m.dataCache, n, m.labels)
   if id == m.labels:
     let pureLit = getTempName(m)
     genStringLiteralDataOnlyV2(m, n.strVal, pureLit, isConst)
@@ -42,7 +42,7 @@ proc genStringLiteralV2(m: BModule; n: PNode; isConst: bool): Rope =
           rope(if isConst: "const" else: "")])
 
 proc genStringLiteralV2Const(m: BModule; n: PNode; isConst: bool): Rope =
-  let id = nodeTableTestOrSet(m.dataCache, n, m.labels)
+  let id = getOrPut(m.dataCache, n, m.labels)
   var pureLit: Rope
   if id == m.labels:
     pureLit = getTempName(m)
diff --git a/compiler/backend/cgen.nim b/compiler/backend/cgen.nim
index ad48cfcd757..17e6b452e4b 100644
--- a/compiler/backend/cgen.nim
+++ b/compiler/backend/cgen.nim
@@ -27,7 +27,6 @@ import
     types,
     typesrenderer,
     wordrecg,
-    treetab,
     renderer,
     lineinfos,
     astmsgs,
@@ -1233,7 +1232,7 @@ proc rawNewModule*(g: BModuleList; module: PSym, filename: AbsoluteFile): BModul
   result.module = module
   result.typeInfoMarker = initTable[SigHash, Rope]()
   result.sigConflicts = initCountTable[SigHash]()
-  initNodeTable(result.dataCache)
+  result.dataCache = initTable[ConstrTree, int]()
   result.typeStack = @[]
   result.typeNodesName = getTempName(result)
   # no line tracing for the init sections of the system module so that we
diff --git a/compiler/backend/cgendata.nim b/compiler/backend/cgendata.nim
index d24d9ce8179..de632d8bc90 100644
--- a/compiler/backend/cgendata.nim
+++ b/compiler/backend/cgendata.nim
@@ -11,6 +11,7 @@
 
 import
   std/[
+    hashes,
     intsets,
     tables,
     sets
@@ -18,7 +19,8 @@ import
   compiler/ast/[
     ast,
     lineinfos,
-    ndi
+    ndi,
+    types
   ],
   compiler/modules/[
     modulegraphs
@@ -28,6 +30,7 @@ import
   ],
   compiler/utils/[
     containers,
+    idioms,
     ropes,
     pathutils
   ]
@@ -88,6 +91,11 @@ type
                               ## ``DiscoverData`` and ``ProcLoc``
     params*: seq[TLoc]        ## the locs of the parameters
 
+  ConstrTree* = distinct PNode
+    ## A ``PNode`` tree that represents a literal primitive/aggregate value
+    ## construction expression. A ``distinct`` alias for ``PNode`` is used
+    ## such that special equality and hash operations can be attached.
+
   TLabel* = Rope              ## for the C generator a label is just a rope
   TCFileSection* = enum       ## the sections a generated C file consists of
     cfsMergeInfo,             ## section containing merge information
@@ -242,7 +250,9 @@ type
     typeInfoMarker*: TypeCache ## needed for generating type information
     typeInfoMarkerV2*: TypeCache
     typeStack*: TTypeSeq      ## used for type generation
-    dataCache*: TNodeTable
+    dataCache*: Table[ConstrTree, int] ## maps a value construction
+                              ## expression to the label of the C constant
+                              ## created for it
     typeNodes*: int ## used for type info generation
     typeNodesName*: Rope ## used for type info generation
     labels*: Natural          ## for generating unique module-scope names
@@ -332,4 +342,70 @@ func contains*[T](m: SymbolMap[T], sym: PSym): bool {.inline.} =
 
 iterator items*[T](m: SymbolMap[T]): lent T =
   for it in m.store.items:
-    yield it
\ No newline at end of file
+    yield it
+
+proc hash(n: ConstrTree): Hash =
+  ## Computes a hash over the structure of a tree (`n`). The hash function is
+  ## intended to be used with ``Table``, so two different trees are not
+  ## guaranteed to produce a different hash, but the same hash *must* be
+  ## produced for two structurally equal trees.
+  proc hashTree(n: PNode): Hash =
+    result = ord(n.kind)
+    case n.kind
+    of nkEmpty, nkNilLit, nkType:
+      discard
+    of nkSym:
+      result = result !& n.sym.id
+    of nkIntKinds:
+      result = result !& hash(n.intVal)
+    of nkFloatKinds:
+      # we'll be comparing the bit patterns later on, meaning that
+      # they're what we have to compute the hash for
+      result = result !& hash(cast[BiggestInt](n.floatVal))
+    of nkStrKinds:
+      result = result !& hash(n.strVal)
+    of nkWithSons:
+      for i in 0..<n.len:
+        result = result !& hashTree(n[i])
+    of nkNone, nkIdent, nkError:
+      unreachable()
+    result = !$result
+
+  result = hashTree(PNode(n))
+
+proc `==`(a, b: ConstrTree): bool =
+  ## Computes and returns whether `a` and `b` are structurally equal *and*
+  ## have equal types.
+  proc treesEquivalent(a, b: PNode): bool =
+    if a == b:
+      result = true
+    elif a.kind == b.kind:
+      case a.kind
+      of nkEmpty, nkNilLit, nkType:
+        result = true
+      of nkSym:
+        result = a.sym.id == b.sym.id
+      of nkIntKinds:
+        result = a.intVal == b.intVal
+      of nkFloatKinds:
+        result = cast[BiggestInt](a.floatVal) == cast[BiggestInt](b.floatVal)
+      of nkStrKinds:
+        result = a.strVal == b.strVal
+      of nkWithSons:
+        if a.len == b.len:
+          for i in 0..<a.len:
+            if not treesEquivalent(a[i], b[i]): return
+          result = true
+      of nkNone, nkIdent, nkError:
+        unreachable()
+
+      # we also want equal types:
+      if result:
+        result = sameTypeOrNil(a.typ, b.typ)
+
+  treesEquivalent(PNode(a), PNode(b))
+
+proc getOrPut*(t: var Table[ConstrTree, int], n: PNode, label: int): int =
+  ## Fetches the label for the given data AST, or adds the AST + label to the
+  ## table first if they're not present yet.
+  mgetOrPut(t, ConstrTree(n), label)
\ No newline at end of file
diff --git a/tests/lang_exprs/tlifted_constants_with_float.nim b/tests/lang_exprs/tlifted_constants_with_float.nim
new file mode 100644
index 00000000000..111a15048d1
--- /dev/null
+++ b/tests/lang_exprs/tlifted_constants_with_float.nim
@@ -0,0 +1,47 @@
+discard """
+  targets: "c js vm"
+  description: '''
+    Regression test for two constant array construction expressions being
+    lifted into the *same* constant, despite their float contents having
+    different bit representations
+  '''
+"""
+
+import std/math
+
+type Obj = object
+  x: float
+
+proc test() =
+  # use a wrapper procedure in order to test with locals, and not globals
+  var
+    a = [-0.0, 0.0]
+    b = [0.0, 0.0]
+
+  # the array values are compile-time known, meaning that they can be lifted
+  # into constants, but they must not be erroneously collapsed into a single
+  # one
+  doAssert classify(a[0]) == fcNegZero
+  doAssert classify(a[1]) == fcZero
+  doAssert classify(b[0]) == fcZero
+  doAssert classify(b[1]) == fcZero
+
+  # for completeness, also test with tuples and objects:
+  var
+    c = (-0.0, 0.0)
+    d = (0.0, 0.0)
+
+  doAssert classify(c[0]) == fcNegZero
+  doAssert classify(c[1]) == fcZero
+  doAssert classify(d[0]) == fcZero
+  doAssert classify(d[1]) == fcZero
+
+  # test with objects:
+  var
+    e = Obj(x: -0.0)
+    f = Obj(x: 0.0)
+
+  doAssert classify(e.x) == fcNegZero
+  doAssert classify(f.x) == fcZero
+
+test()
\ No newline at end of file