From 16cb5ecf75a84433ce36d009f25123a11e9c85b4 Mon Sep 17 00:00:00 2001 From: zerbina <100542850+zerbina@users.noreply.github.com> Date: Mon, 19 May 2025 18:27:59 +0000 Subject: [PATCH 01/11] skully: fix size/alignment for embedded records Both embedded unions and records had incorrect sizes and alignments computed for them. None of the current passes cared, hence this causing no problems, but the LLVM code generator does care. The size and alignment for embedded records/union is now computed correctly. --- skully/backend.nim | 75 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 53 insertions(+), 22 deletions(-) diff --git a/skully/backend.nim b/skully/backend.nim index fcae2c06..ae13703f 100644 --- a/skully/backend.nim +++ b/skully/backend.nim @@ -308,6 +308,49 @@ proc translateProcType(c; env: TypeEnv, id: TypeId, ignoreClosure: bool, bu) = if desc.callConv(env) == ccClosure and not ignoreClosure: bu.add typeRef(c, env, PointerType) +proc computeSizeAlign(c; env: TypeEnv, typ: Typeid, + withTag = true): (int64, int16) = + ## Computes the size and alignment of an embedded record/union. + # TODO: this shouldn't be our responsiblity! The NimSkull compiler has to + # set the size and alignment for embedded records + let desc = env.headerFor(typ, Lowered) + var size = 0'i64 + var align = 0'i16 + case desc.kind + of tkTaggedUnion: + if withTag: + let tag = env.headerFor(env[env.lookupField(typ, 0)].typ, Lowered) + align = tag.align + size = tag.size(env) + + for _, it in env.fields(desc, 1): + if env.isEmbedded(it.typ): + # an anonymous record; its fields need to be considered too + let (s, a) = computeSizeAlign(c, env, it.typ) + align = max(align, a) + size = max(size, s) + else: + let sub = env.headerFor(it.typ, Canonical) + align = max(align, sub.align) + size = max(size, sub.size(env)) + of tkRecord: + for _, it in env.fields(desc, 0): + # align the offset, then add the size + if env.isEmbedded(it.typ): + let (s, a) = computeSizeAlign(c, env, it.typ) + size = align(size, a) + size += s + align = max(align, a) + else: + let sub = env.headerFor(it.typ, Canonical) + size = align(size, sub.align) + size += sub.size(env) + align = max(align, sub.align) + else: + unreachable() + # don't add any padding at the end + result = (size, align) + proc embedTaggedUnion(c; env: TypeEnv, id: TypeId, bu) = privateAccess(RecField) # required for accessing the field offsets @@ -317,29 +360,16 @@ proc embedTaggedUnion(c; env: TypeEnv, id: TypeId, bu) = let desc = env.headerFor(id, Lowered) tagField = env.lookupField(id, 0) - - # compute the alignment for the union: - # TODO: this needs to be upstreamed into NimSkull - var alignment = 0'i16 - for _, it in env.fields(desc, 1): - let sub = env.headerFor(it.typ, Canonical) - if env.isEmbedded(it.typ): - # an anonymous record; its fields need to be considered too - for _, inner in env.fields(sub): - alignment = max(alignment, env.headerFor(inner.typ, Canonical).align) - else: - alignment = max(alignment, sub.align) - - let offset = align(env[tagField].offset.uint32 + desc.size(env).uint32, - uint32 alignment) - ## the offset of the union within the embedding record + tagDesc = env.headerFor(env[tagField].typ, Lowered) + (size, alignment) = computeSizeAlign(c, env, id, withTag=false) + # the size and alignment of the union (without the tag) + offset = align(env[tagField].offset.uint32 + tagDesc.size(env).uint32, + uint32 alignment) + ## the offset of the union within the embedding record let inner = block: - # XXX: size and alignment are currently ignored, as it's simpler this way - # and no pass inspects these values anyhow (at least at the time of - # writing) var bu = initBuilder(Union) - bu.add node(Immediate, 0) + bu.add node(Immediate, uint32 size) bu.add node(Immediate, uint32 alignment) for _, it in env.fields(desc, 1): if env.isEmbedded(it.typ): @@ -347,12 +377,13 @@ proc embedTaggedUnion(c; env: TypeEnv, id: TypeId, bu) = # IL record types must not be empty; add a dummy field bu.add node(UInt, 1) else: + let (s, a) = computeSizeAlign(c, env, it.typ) # emit the record type for the embedded type, but make sure to # correct the field offsets; they need to be relative to the start # of the union, not to the start of the embedding record var bu2 = initBuilder(Record) - bu2.add node(Immediate, 0) - bu2.add node(Immediate, 0) + bu2.add node(Immediate, s.uint32) + bu2.add node(Immediate, a.uint32) for _, recf in env.fields(env.headerFor(it.typ, Lowered)): bu2.subTree Field: bu2.add node(Immediate, recf.offset.uint32 - offset) From c7ea7f03a844a4c07f270b848d4c8db915d184be Mon Sep 17 00:00:00 2001 From: zerbina <100542850+zerbina@users.noreply.github.com> Date: Mon, 19 May 2025 18:27:59 +0000 Subject: [PATCH 02/11] skully: support `nil` in constant data --- skully/backend.nim | 2 ++ 1 file changed, 2 insertions(+) diff --git a/skully/backend.nim b/skully/backend.nim index ae13703f..3f8e51c2 100644 --- a/skully/backend.nim +++ b/skully/backend.nim @@ -669,6 +669,8 @@ proc genConst(c; env; tree; n; dest: Expr, stmts) = putIntoDest node(FloatVal, c.lit.pack(env.getFloat(tree[n].number))) of mnkProcVal: putIntoDest node(ProcVal, c.registerProc(tree[n].prc)) + of mnkNilLit: + putIntoDest node(Nil) of mnkArrayConstr: for i, it in tree.args(n): let nDest = makeExpr tree[it].typ: From b473125884e1a796740362b874f7ac44572af402 Mon Sep 17 00:00:00 2001 From: zerbina <100542850+zerbina@users.noreply.github.com> Date: Mon, 19 May 2025 18:28:00 +0000 Subject: [PATCH 03/11] skully: fix wrong default assignment code Pointer-like variables were initialized with integer values. --- skully/backend.nim | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/skully/backend.nim b/skully/backend.nim index 3f8e51c2..a8eda733 100644 --- a/skully/backend.nim +++ b/skully/backend.nim @@ -1045,11 +1045,14 @@ proc getTypeInfoV2(c; env: var MirEnv, typ: TypeId): Expr = proc genDefault(c; env: var MirEnv; dest: Expr, typ: TypeId, bu) = let typ = env.types.canonical(typ) case env.types.headerFor(typ, Lowered).kind - of tkBool, tkChar, tkInt, tkUInt, tkRef, tkPtr, tkVar, tkLent, tkPointer: + of tkBool, tkChar, tkInt, tkUInt: genAsgn(dest, makeExpr(@[node(IntVal, c.lit.pack(0))], typ), bu) of tkFloat: genAsgn(dest, makeExpr(@[node(FloatVal, c.lit.pack(0.0))], typ), bu) + of tkRef, tkPtr, tkVar, tkLent, tkPointer, tkCstring, tkProc: + genAsgn(dest, makeExpr(@[node(Nil)], typ), bu) else: + # TODO: for seqs, closures, and openarrays, set the fields directly let size = env.types.headerFor(typ, Lowered).size(env.types) bu.subTree Clear: takeAddr(dest, bu) From 5ebd7388d67cb3930cd8540f792f3d1183629dd8 Mon Sep 17 00:00:00 2001 From: zerbina <100542850+zerbina@users.noreply.github.com> Date: Mon, 19 May 2025 18:28:00 +0000 Subject: [PATCH 04/11] skully: fix numeric conversion translation --- skully/backend.nim | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/skully/backend.nim b/skully/backend.nim index a8eda733..a1da4b77 100644 --- a/skully/backend.nim +++ b/skully/backend.nim @@ -1150,10 +1150,10 @@ proc genNumericConv(c; env; val: Expr, to: TypeId, bu) = bu.add typeRef(c, env, val.typ) bu.use val - template sizedConv(widen, narrow: NodeKind) = + template sizedConv(narrow, widen: NodeKind) = if dst.size(env.types) < src.size(env.types): convOp narrow - elif dst.size(env.types) > dst.size(env.types): + elif dst.size(env.types) > src.size(env.types): convOp widen else: # nothing to do @@ -1185,7 +1185,7 @@ proc genNumericConv(c; env; val: Expr, to: TypeId, bu) = of ints, uints: convOp Conv of tkFloat: - sizedConv Promote, Demote + sizedConv Demote, Promote else: unreachable() of tkPtr, tkPointer, tkProc, tkCstring: From 892853e9355eef12b36f8b4bc0a5049a2cfad753 Mon Sep 17 00:00:00 2001 From: zerbina <100542850+zerbina@users.noreply.github.com> Date: Mon, 19 May 2025 18:28:01 +0000 Subject: [PATCH 05/11] skully: fix `mEqProc` translation --- skully/backend.nim | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/skully/backend.nim b/skully/backend.nim index a1da4b77..e5fc9c25 100644 --- a/skully/backend.nim +++ b/skully/backend.nim @@ -1440,7 +1440,7 @@ proc genMagic(c; env: var MirEnv, tree; n; dest: Expr, stmts) = # TODO: use a proper short-circuiting ``and``, not a ``bitand`` wrapAsgn BitAnd: bu.add typeRef(c, env, PointerType) - wrapAsgn Eq: + bu.subTree Eq: bu.add typeRef(c, env, PointerType) bu.subTree Copy: bu.subTree Field: @@ -1450,7 +1450,7 @@ proc genMagic(c; env: var MirEnv, tree; n; dest: Expr, stmts) = bu.subTree Field: lvalue tree.argument(n, 1) bu.add node(Immediate, 0) - wrapAsgn Eq: + bu.subTree Eq: bu.add typeRef(c, env, PointerType) bu.subTree Copy: bu.subTree Field: From a9e91965755b907ed3d119667e612d29490936c6 Mon Sep 17 00:00:00 2001 From: zerbina <100542850+zerbina@users.noreply.github.com> Date: Mon, 19 May 2025 18:28:01 +0000 Subject: [PATCH 06/11] skully: fix `mOrd` translation It was treated as a no-op, while it's actually a numeric conversion. --- skully/backend.nim | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/skully/backend.nim b/skully/backend.nim index e5fc9c25..ca6f108a 100644 --- a/skully/backend.nim +++ b/skully/backend.nim @@ -1510,10 +1510,7 @@ proc genMagic(c; env: var MirEnv, tree; n; dest: Expr, stmts) = bu.add typeRef(c, env, tree[n].typ) value(tree.argument(n, 0)) value(tree.argument(n, 1)) - of mOrd: - wrapAsgn: - value(tree.argument(n, 0)) - of mChr: + of mOrd, mChr: # it's a conversion, nothing more let input = c.gen(env, tree, NodePosition(tree.argument(n, 0)), true) wrapAsgn: From 7d821bf0f4d5f70606e5a94e9e172b2f445d47b5 Mon Sep 17 00:00:00 2001 From: zerbina <100542850+zerbina@users.noreply.github.com> Date: Mon, 19 May 2025 18:28:02 +0000 Subject: [PATCH 07/11] skully: fix string comparison translation --- skully/backend.nim | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/skully/backend.nim b/skully/backend.nim index ca6f108a..0b4f39c3 100644 --- a/skully/backend.nim +++ b/skully/backend.nim @@ -1596,14 +1596,14 @@ proc genMagic(c; env: var MirEnv, tree; n; dest: Expr, stmts) = bu.subTree Call: bu.add compilerProc(c, env, "cmpStrings") value(tree.argument(n, 0)) - bu.add node(IntVal, 0) + bu.add node(IntVal, 0) of mLtStr: wrapAsgn Lt: bu.add typeRef(c, env, env.types.sizeType) bu.subTree Call: bu.add compilerProc(c, env, "cmpStrings") value(tree.argument(n, 0)) - bu.add node(IntVal, 0) + bu.add node(IntVal, 0) of mFinished: # the status field is stored at the start of the env object, load it and # test whether the value is < 0 From 1748108a53b290044e60b856e368da359ebb44f1 Mon Sep 17 00:00:00 2001 From: zerbina <100542850+zerbina@users.noreply.github.com> Date: Mon, 19 May 2025 18:28:02 +0000 Subject: [PATCH 08/11] skully: fix `mDestroy` translation --- skully/backend.nim | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/skully/backend.nim b/skully/backend.nim index 0b4f39c3..0832a3af 100644 --- a/skully/backend.nim +++ b/skully/backend.nim @@ -1820,7 +1820,7 @@ proc genMagic(c; env: var MirEnv, tree; n; dest: Expr, stmts) = bu.subTree Copy: bu.subTree Field: lvalue(tree.argument(n, 0)) - bu.add node(Immediate, 0) + bu.add node(Immediate, 1) bu.add node(Nil) bu.goto then bu.goto els From e3f056ab7326f94b8ad35f30ea0a7884033a259e Mon Sep 17 00:00:00 2001 From: zerbina <100542850+zerbina@users.noreply.github.com> Date: Mon, 19 May 2025 18:28:03 +0000 Subject: [PATCH 09/11] skully: use `Nil` instead of zero There were various places missed when adjusting skully to the new pointer types. --- skully/backend.nim | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/skully/backend.nim b/skully/backend.nim index 0832a3af..fcbc5514 100644 --- a/skully/backend.nim +++ b/skully/backend.nim @@ -1920,7 +1920,7 @@ proc genMagic(c; env: var MirEnv, tree; n; dest: Expr, stmts) = bu.subTree Eq: bu.add typeRef(c, env, e.typ) bu.use e - bu.add node(IntVal, c.lit.pack(0)) + bu.add node(Nil) bu.goto els bu.goto then stmts.join then @@ -2094,7 +2094,7 @@ proc translateExpr(c; env: var MirEnv, tree; n; dest: Expr, stmts) = bu.subTree Field: lvalue callee bu.add node(Immediate, 1) - bu.add node(IntVal, c.lit.pack(0)) + bu.add node(Nil) bu.goto els bu.goto then stmts.join then @@ -2300,7 +2300,7 @@ proc translateExpr(c; env: var MirEnv, tree; n; dest: Expr, stmts) = bu.subTree Field: bu.useLvalue dest bu.add node(Immediate, 0) - bu.add node(IntVal, c.lit.pack(0)) + bu.add node(Nil) stmts.addStmt Asgn: bu.subTree Field: bu.useLvalue dest @@ -2616,7 +2616,7 @@ proc translateStmt(env: var MirEnv, tree; n; stmts; c) = of mnkContinue: guardActive() stmts.addStmt Raise: - bu.add node(IntVal, 0) + bu.add node(Nil) c.genExit(tree, tree.child(n, 0), bu) c.prc.active = false of mnkRaise: @@ -2624,7 +2624,7 @@ proc translateStmt(env: var MirEnv, tree; n; stmts; c) = # NimSkull exceptions are managed separately; there's nothing to pass # along to ``Raise`` stmts.addStmt Raise: - bu.add node(IntVal, 0) + bu.add node(Nil) c.genExit(tree, tree.child(n, 0), bu) c.prc.active = false of mnkEmit, mnkAsm: From 2aa6c5ae9290d039a3d7379fd9a7145f43f7fea7 Mon Sep 17 00:00:00 2001 From: zerbina <100542850+zerbina@users.noreply.github.com> Date: Mon, 19 May 2025 18:28:03 +0000 Subject: [PATCH 10/11] skully: fix `mnkCast` translation * properly handle imported types * consider float types (which also cannot be truncated/extended) --- skully/backend.nim | 44 +++++++++++++++++++++++++------------------- 1 file changed, 25 insertions(+), 19 deletions(-) diff --git a/skully/backend.nim b/skully/backend.nim index fcbc5514..33f61b43 100644 --- a/skully/backend.nim +++ b/skully/backend.nim @@ -2368,8 +2368,14 @@ proc translateExpr(c; env: var MirEnv, tree; n; dest: Expr, stmts) = c.genSetElem(env, tree, it, tree[n].typ, bu) c.genIncl(env, dest, e, stmts) of mnkCast: - let dst = env.types.canonical(tree[n].typ) - let src = env.types.canonical(tree[n, 0].typ) + proc canon(env: TypeEnv, id: TypeId): TypeId = + ## Skips to the non-imported canonical type. + result = env.canonical(id) + if env.headerFor(result, Lowered).kind == tkImported: + result = canon(env, env.headerFor(result, Lowered).elem) + + let dst = env.types.canon(tree[n].typ) + let src = env.types.canon(tree[n, 0].typ) let tdst = c.genType(env.types, dst) let tsrc = c.genType(env.types, src) if tdst == tsrc: @@ -2385,9 +2391,9 @@ proc translateExpr(c; env: var MirEnv, tree; n; dest: Expr, stmts) = takeAddr tree.child(n, 0) bu.add node(IntVal, c.lit.pack(size)) else: - template isPointer(id: TypeId): bool = + template isInt(id: TypeId): bool = env.types.headerFor(id, Lowered).kind in - {tkPointer, tkProc, tkRef, tkPtr, tkCstring} + {tkInt, tkUInt, tkBool, tkChar} let a = env.types.headerFor(dst, Lowered).size(env.types) let b = env.types.headerFor(src, Lowered).size(env.types) @@ -2400,29 +2406,29 @@ proc translateExpr(c; env: var MirEnv, tree; n; dest: Expr, stmts) = value tree.child(n, 0) else: let op = if a < b: Trunc else: Zext - # pointer values cannot be truncated/extended directly -- the operation - # has to happen on a uint - if isPointer(src): - wrapAsgn op: - bu.add typeRef(c, env, dst) - bu.add typeRef(c, env, env.types.usizeType) + var val = c.gen(env, tree, tree.child(n, 0), true) + # non-integer values cannot be truncated/extended directly; the value + # has to be bitcast into an int first + if not isInt(src): + val = makeExpr env.types.usizeType: bu.subTree Reinterp: bu.add typeRef(c, env, env.types.usizeType) bu.add typeRef(c, env, src) - value tree.child(n, 0) - elif isPointer(dst): + bu.use val + + if isInt(dst): + wrapAsgn op: + bu.add typeRef(c, env, dst) + bu.add typeRef(c, env, val.typ) + bu.use val + else: wrapAsgn Reinterp: bu.add typeRef(c, env, dst) bu.add typeRef(c, env, env.types.usizeType) bu.subTree op: bu.add typeRef(c, env, env.types.usizeType) - bu.add typeRef(c, env, src) - value tree.child(n, 0) - else: - wrapAsgn op: - bu.add typeRef(c, env, dst) - bu.add typeRef(c, env, dst) - value tree.child(n, 0) + bu.add typeRef(c, env, val.typ) + bu.use val else: unreachable() From 843c659b8f19bd8cf7f1e6c53976912c3abfb300 Mon Sep 17 00:00:00 2001 From: zerbina <100542850+zerbina@users.noreply.github.com> Date: Mon, 19 May 2025 18:28:04 +0000 Subject: [PATCH 11/11] skully: update some comments --- skully/backend.nim | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/skully/backend.nim b/skully/backend.nim index 33f61b43..e2be3ce9 100644 --- a/skully/backend.nim +++ b/skully/backend.nim @@ -998,7 +998,8 @@ proc translateValue(c; env: MirEnv, tree: MirTree, n: NodePosition, emit(c, env, tree, tree.child(n, 0), diff, b, bu) else: if env.types.headerFor(b, Lowered).kind in PointerLike: - # there are no pointer types in the IL; nothing to do + # all MIR pointer types map to the same IL pointer type; + # nothing special to do recurse(tree.child(n, 0), wantValue) else: assert not wantValue @@ -1429,7 +1430,7 @@ proc genMagic(c; env: var MirEnv, tree; n; dest: Expr, stmts) = of mEqProc: let typ = env.types.canonical(tree[tree.argument(n, 0)].typ) if env.types.headerFor(typ, Lowered).kind == tkProc: - # simple integer equality suffices + # simple pointer equality suffices wrapAsgn Eq: bu.add typeRef(c, env, typ) value(tree.argument(n, 0))