From ce802883580301317031d463e59612b4600d28fe Mon Sep 17 00:00:00 2001
From: zerbina <100542850+zerbina@users.noreply.github.com>
Date: Wed, 19 Jul 2023 14:03:17 +0000
Subject: [PATCH] cgen: fix performance regression with `x in {...}`

Summary
=======

Fix a regression in the C code generator that led to slightly less
efficient code being generated for `x in set` where set is a constant
set construction expression.

Details
=======

The decision of whether to emit set construction code or to use an
`||` (or) chain of comparisons was still based on the presence of the
`nfAllConst` flag, but node flags don't reach the code generators ever
since the introduction of the MIR and `astgen`, meaning that the `||`
chain was always chosen for set literals with less than 8 elements.

Testing for the flag is replaced with a call to `isDeepConstExpr`,
restoring the originally intended behaviour (i.e., emit set construction
code for sets with a size-in-bytes < `sizeof(int)`).

In addition, the other remaining usage of `nfAllConst` in
`ccgexprs.genSetConstr` is removed completely: fully constant
construction expression are already handled by its single callsite
(`ccgexprs.expr`).
---
 compiler/backend/ccgexprs.nim         |  6 ++----
 tests/ccgbugs/tsmall_set_contains.nim | 14 ++++++++++++++
 2 files changed, 16 insertions(+), 4 deletions(-)
 create mode 100644 tests/ccgbugs/tsmall_set_contains.nim

diff --git a/compiler/backend/ccgexprs.nim b/compiler/backend/ccgexprs.nim
index 1acf76957f5..c766ad5dd76 100644
--- a/compiler/backend/ccgexprs.nim
+++ b/compiler/backend/ccgexprs.nim
@@ -1451,7 +1451,7 @@ proc fewCmps(conf: ConfigRef; s: PNode): bool =
   # this function estimates whether it is better to emit code
   # for constructing the set or generating a bunch of comparisons directly
   if s.kind != nkCurly: return false
-  if (getSize(conf, s.typ) <= conf.target.intSize) and (nfAllConst in s.flags):
+  if (getSize(conf, s.typ) <= conf.target.intSize) and isDeepConstExpr(s):
     result = false            # it is better to emit the set generation code
   elif elemType(s.typ).kind in {tyInt, tyInt16..tyInt64}:
     result = true             # better not emit the set if int is basetype!
@@ -1952,9 +1952,7 @@ proc genSetConstr(p: BProc, e: PNode, d: var TLoc) =
   # incl(tmp, d); incl(tmp, e); inclRange(tmp, f, g);
   var
     a, b, idx: TLoc
-  if nfAllConst in e.flags:
-    putIntoDest(p, d, e, genSetNode(p, e))
-  else:
+  if true:
     if d.k == locNone: getTemp(p, e.typ, d)
     if getSize(p.config, e.typ) > 8:
       # big set:
diff --git a/tests/ccgbugs/tsmall_set_contains.nim b/tests/ccgbugs/tsmall_set_contains.nim
new file mode 100644
index 00000000000..a3fa5396f9a
--- /dev/null
+++ b/tests/ccgbugs/tsmall_set_contains.nim
@@ -0,0 +1,14 @@
+discard """
+  targets: "c"
+  description: '''
+    Ensure that no '||' chain is emitted for `contains` where the set operand
+    is a small set literal
+  '''
+  action: compile
+  ccodecheck: "\\i !@('||')"
+"""
+
+type T = range[0..7]
+
+var elem = T(1)
+doAssert contains({T(1), T(3)}, elem)
\ No newline at end of file