Skip to content

Commit

Permalink
cgen: fix performance regression with x in {...}
Browse files Browse the repository at this point in the history
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`).
  • Loading branch information
zerbina committed Jul 19, 2023
1 parent f1cd43c commit ce80288
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 4 deletions.
6 changes: 2 additions & 4 deletions compiler/backend/ccgexprs.nim
Original file line number Diff line number Diff line change
Expand Up @@ -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!
Expand Down Expand Up @@ -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:
Expand Down
14 changes: 14 additions & 0 deletions tests/ccgbugs/tsmall_set_contains.nim
Original file line number Diff line number Diff line change
@@ -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)

0 comments on commit ce80288

Please sign in to comment.