-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
codegen: optimize single-operand CgNode
s
#836
codegen: optimize single-operand CgNode
s
#836
Conversation
Summary ======= Add a dedicated `CgNode` branch for expression-like nodes that always have a single operand. This reduces the amount of allocations and access indirections, and it also makes the code more self-describing (`.operand` now vs. the previous `[0]`). Details ======= For now, only expression-like nodes use the `operand` branch, but some statement nodes (for example, `mnkRepeat`) could also be considered for using the `operand` branch in the future. The set previously denoting all atoms (`cnkWithoutItem`) is renamed to `cnkAtoms` and the usage sites are adjusted to take the new meaning into account: only traversing `mnkWithItem` nodes is not enough anymore. The `cnkCast`, `cnkConv`, and `cnkHiddenConv` nodes also use the `operand` branch. Them using two subnodes was inherited from `PNode`, but it's not something that's needed for `CgNode` IR. `cgirgen` is changed to create `operand`-nodes where required, and all `[0]` and `[1]` (for `cnkCast|cnkConv|cnkHiddenConv`) access for the changed nodes is replaced with `.operand`. Allocations stats ----------------- The accumulated allocation stats (using `-d:nimAllocStats) of `generateIR` when compiling the compiler at 48a07fb are: |version|allocs|deallocs| |:------|:-----|:-------| |this|3162007|deallocs|752474| |48a07fbfc2|3320434|758663|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the clear name over 'idiomatic' traversal is much more pleasant to read, with storage benefits to boot. :D
@@ -205,8 +205,11 @@ func underlyingLoc(n: CgNode): PSym = | |||
var root {.cursor.} = n | |||
# skip nodes that don't change the location until we arrive at either one | |||
# that does, or a symbol | |||
while root.kind in {cnkConv, cnkStmtListExpr}: | |||
root = root.lastSon | |||
while true: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while true: | |
while root.kind in {cnkConv, cnkStmtListExpr}: |
at first i thought this might be a good idea, but it ends up meaning two places have to be updated correctly in order for things to work. thinking about it a bit further and i realize that i'm suggesting this to avoid leaving infinite loops around.
separate from this PR, do you think we should have an idiom, like so?
template loopForever(body: untyped) =
var i = 0
const maxIterations = 1000
while i < maxIterations:
body
inc i
doAssert i < maxIterations, "fixed point calculation exceeded maximum iteration attempts"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, this did bother me a bit too. A while true
is always a bit meh, though I think that, just like you said, not duplicating the stats is the better choice here.
Regarding the idiom, I'm a bit unsure. To me, it feels like a work-around to missing static analysis -- essentially, the compiler could (and I think should) detect while
loops where a control-flow path exists on which no progress is made (i.e., no state is modified that is used in conditions guarding loop exits).
Adding the idiom likely still makes sense, but figuring out a maxIterations
value that works well across different use cases could be hard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the input. Static analysis + some wise API design choices is definitely the way to go. In the meantime, we can make the maxIteration
a parameter (tacked on the end so it doesn't always have to be specified).
Prevents unintended nodes reaching into the procedure on accident.
/merge |
Merge requested by: @saem Contents after the first section break of the PR description has been removed and preserved below:
|
Summary
Add a dedicated
CgNode
branch for expression-like nodes that alwayshave a single operand. This reduces the amount of allocations and access
indirections, and it also makes the code more self-describing
(
.operand
now vs. the previous[0]
).Details
For now, only expression-like nodes use the
operand
branch, but somestatement nodes (for example,
mnkRepeat
) could also be considered forusing the
operand
branch in the future. The set previously denotingall atoms (
cnkWithoutItem
) is renamed tocnkAtoms
and the usagesites are adjusted to take the new meaning into account: only traversing
mnkWithItem
nodes is not enough anymore.The
cnkCast
,cnkConv
, andcnkHiddenConv
nodes also use theoperand
branch. Them using two subnodes was inherited fromPNode
,but it's not something that's needed for
CgNode
IR.cgirgen
is changed to createoperand
-nodes where required, and all[0]
and[1]
(forcnkCast|cnkConv|cnkHiddenConv
) access for thechanged nodes is replaced with
.operand
.Allocation Stats
The accumulated allocation stats (using
-d:nimAllocStats
) ofgenerateIR
when compiling the compiler at 48a07fb are: