internal: move MIR generation DSL to `mirconstr` #793

zerbina · 2023-07-10T21:31:53Z

Summary

Move the routines and types that make up the DSL to the mirconstr
module and use the DSL in both the injectdestructors and mirbridge
module.

Apart from making the MIR code generation in both modules less complex,
manual, and error-prone, this is also significant progress towards the
MIR code coming out of the injectdestructors pass having proper type
information, which is a requirement for having MIR passes that run after
injectdestructors.

Details

move the EValue and ChainEnd types to mirconstr
move the atomic DSL operands to mirconstr. The notOp, modify,
outOp, and tupleAccess operands stay in mirgen
add wrapper templates for DSL operands that mirgen still calls with
a TCtx instance instead of a MirNodeSeq

As part of moving them, some operand routines that had a 'gen' prefix
are changed to not have it, making the routine names more consistent.

In addition, some cleanup in mirconstr is performed, and the DSL
extended a bit:

|=>, =>|, and previous are removed. They were unused and didn't
fit in with the overall design
the emit, symbol, and opParam operands are added, all of which
are only useful outside of mirgen

Another addition is the predicate meta operand, which makes it
possible to write chains where an operand is conditionally excluded:

chain(...): a() => predicate(cond) => b() => c()

Here, b() is only evaluated if cond evaluates to 'true'. Using two
chains plus an in-between 'if' statement was previously necessary to
achieve the same.

Finally, documentation about the DSL is added, and the
injectdestructors and mirbridge modules are changed to use the DSL
for generating MIR code. The logic from the genInjectedSink procedure
was previously duplicated into genSinkFromTemporary, but this is now
fixed.

It's self-contained and doesn't depend on the rest of `mirgen`. The `notOp`, `modify`, `outOp`, and `tupleAccess` operators are not moved -- they're not atoms like the others. For convenience and to keep the set of changes smaller, `mirgen` uses wrapper templates around some of the DSL routines. Apart from some very small documentation fixes, the moved routines are not modified.

They're useful in non-`mirgen` contexts (e.g., `injectdestructors`, which is going to be changed to use `mirconstr`).

It's useful for conditionally excluding chain operands based on some run-time value. For example: ```nim var x = eval: a() if someBool: x = eval: x => b() chain: x => c() ``` can with `cond` be replaced with: ```nim chain: a() => predicate(someBool) => b() => c() ```

They were unused and also didn't fit with the overall design.

Remove the 'gen' prefixes from the chain DSL operands that have them. `genVoid` cannot be renamed to `void`, as that would make the operator not usable in the DSL -- it is instead renamed to `voidOut`.

This shrinks the code quite a bit, and also fixes the problems with nodes staying untyped.

saem · 2023-07-10T21:45:15Z

Updated the PR description, under the details section, the second bullet point said that notOp etc stayed in mirconstr, which isn't the case, updated to mirgen.

zerbina · 2023-07-10T21:49:12Z

Yep, the bullet point was wrong, thank you.

saem

DSL definitely makes it more compact, while keeping the tree structure relatively straightforward to follow.

saem · 2023-07-10T22:10:56Z

compiler/mir/mirconstr.nim

+func name*(s: var MirNodeSeq, val: EValue) =
+  s.add MirNode(kind: mnkName, typ: val.typ)
+
+func voidOut*(s: var MirNodeSeq, val: EValue) =


Suggested change

func voidOut*(s: var MirNodeSeq, val: EValue) =

func voidOut*(s: var MirNodeSeq, val: EValue) =

## end the mir input expression as a void-expression

not sure I have that quite right, this is from what I could gather based on usage. initially I was seeing if I could come up with a better name, settled on a doc string might help. (soft suggestion)

Yup, that sounds correct. I added it plus some additional elaboration.

In general, the 'void' operator/node acts as both a discard and as a delimiter for expressions ('call' operators that yield nothing also need to be followed by 'void').

zerbina · 2023-07-12T13:19:07Z

Apologies for the long time it took me to respond, I didn't manage to get any work done yesterday.

saem · 2023-07-12T17:39:54Z

Apologies for the long time it took me to respond, I didn't manage to get any work done yesterday.

no worries. this is a fun project and I don't have any expectations; some days it's not meant to be.

saem · 2023-07-12T17:41:11Z

hand slipped on the trackpad and I clicked the wrong button (close with comment). 🤦🏽

saem · 2023-07-12T19:37:43Z

/merge

github-actions · 2023-07-12T19:38:09Z

Merge requested by: @saem

Contents after the first section break of the PR description has been removed and preserved below:

Notes for Reviewers

there are still some places in injectdestructors where nodes are missing type information

I figured that combining both moving the DSL and changing injectdestructors into a single PR made sense, as it highlights why the move makes sense, but I can also split the PR up if wanted

zerbina added 8 commits July 10, 2023 17:06

mirconstr: add emit, symbol, and opParam

f8b10a7

They're useful in non-`mirgen` contexts (e.g., `injectdestructors`, which is going to be changed to use `mirconstr`).

mirconstr: remove |=>, =>|, and previous

25cd0ba

They were unused and also didn't fit with the overall design.

mirconstr: make the names consistent

5c25c56

Remove the 'gen' prefixes from the chain DSL operands that have them. `genVoid` cannot be renamed to `void`, as that would make the operator not usable in the DSL -- it is instead renamed to `voidOut`.

injectdestructors: use the chain DSL

bee5e6d

This shrinks the code quite a bit, and also fixes the problems with nodes staying untyped.

mirbridge: use the chain DSL

cd63a8c

mirconstr: add documentation

4b313b7

zerbina added refactor Implementation refactor compiler General compiler tag simplification Removal of the old, unused, unnecessary or un/under-specified language features. labels Jul 10, 2023

zerbina added this to the MIR phase milestone Jul 10, 2023

saem approved these changes Jul 10, 2023

View reviewed changes

mirconstr: document voidOut

3746c1c

saem closed this Jul 12, 2023

saem reopened this Jul 12, 2023

chore-runner bot added this pull request to the merge queue Jul 12, 2023

Merged via the queue into nim-works:devel with commit a5bf48e Jul 12, 2023
36 checks passed

zerbina deleted the mirgen-move-dsl-to-mirconstr branch July 13, 2023 18:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal: move MIR generation DSL to `mirconstr` #793

internal: move MIR generation DSL to `mirconstr` #793

zerbina commented Jul 10, 2023 •

edited by github-actions bot

Loading

saem commented Jul 10, 2023

zerbina commented Jul 10, 2023

saem left a comment

saem Jul 10, 2023

zerbina Jul 12, 2023

zerbina commented Jul 12, 2023

saem commented Jul 12, 2023

saem commented Jul 12, 2023

saem commented Jul 12, 2023

github-actions bot commented Jul 12, 2023

Notes for Reviewers

	func voidOut*(s: var MirNodeSeq, val: EValue) =
	func voidOut*(s: var MirNodeSeq, val: EValue) =
	## end the mir input expression as a void-expression

internal: move MIR generation DSL to mirconstr #793

internal: move MIR generation DSL to mirconstr #793

Conversation

zerbina commented Jul 10, 2023 • edited by github-actions bot Loading

Summary

Details

saem commented Jul 10, 2023

zerbina commented Jul 10, 2023

saem left a comment

Choose a reason for hiding this comment

saem Jul 10, 2023

Choose a reason for hiding this comment

zerbina Jul 12, 2023

Choose a reason for hiding this comment

zerbina commented Jul 12, 2023

saem commented Jul 12, 2023

saem commented Jul 12, 2023

saem commented Jul 12, 2023

github-actions bot commented Jul 12, 2023

Notes for Reviewers

internal: move MIR generation DSL to `mirconstr` #793

internal: move MIR generation DSL to `mirconstr` #793

zerbina commented Jul 10, 2023 •

edited by github-actions bot

Loading