-
Notifications
You must be signed in to change notification settings - Fork 40
Added support for Tuples #374
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
base: hkmc2
Are you sure you want to change the base?
Conversation
LPTK
left a comment
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.
Neat! This looks well done to me. WDYT @Derppening?
hkmc2/shared/src/main/scala/hkmc2/codegen/wasm/text/WatBuilder.scala
Outdated
Show resolved
Hide resolved
hkmc2/shared/src/main/scala/hkmc2/codegen/wasm/text/WatBuilder.scala
Outdated
Show resolved
Hide resolved
hkmc2/shared/src/main/scala/hkmc2/codegen/wasm/text/WatBuilder.scala
Outdated
Show resolved
Hide resolved
hkmc2/shared/src/main/scala/hkmc2/codegen/wasm/text/WatBuilder.scala
Outdated
Show resolved
Hide resolved
hkmc2/shared/src/main/scala/hkmc2/codegen/wasm/text/WatBuilder.scala
Outdated
Show resolved
Hide resolved
Derppening
left a comment
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.
Apart from some fields that appear to be better located in Ctx than WatBuilder, the rest looks good to me.
Please do add documentation where possible, including private members.
hkmc2/shared/src/main/scala/hkmc2/codegen/wasm/text/WatBuilder.scala
Outdated
Show resolved
Hide resolved
hkmc2/shared/src/main/scala/hkmc2/codegen/wasm/text/WatBuilder.scala
Outdated
Show resolved
Hide resolved
hkmc2/shared/src/main/scala/hkmc2/codegen/wasm/text/Instructions.scala
Outdated
Show resolved
Hide resolved
hkmc2/shared/src/main/scala/hkmc2/codegen/wasm/text/WatBuilder.scala
Outdated
Show resolved
Hide resolved
hkmc2/shared/src/main/scala/hkmc2/codegen/wasm/text/WatBuilder.scala
Outdated
Show resolved
Hide resolved
hkmc2/shared/src/main/scala/hkmc2/codegen/wasm/text/WatBuilder.scala
Outdated
Show resolved
Hide resolved
hkmc2/shared/src/main/scala/hkmc2/codegen/wasm/text/WatBuilder.scala
Outdated
Show resolved
Hide resolved
Derppening
left a comment
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.
Please address the comments and also add some brief documentation for the private functions.
hkmc2/shared/src/main/scala/hkmc2/codegen/wasm/text/WatBuilder.scala
Outdated
Show resolved
Hide resolved
|
|
||
| private def tupleArrayGet( | ||
| tupleExpr: Expr, | ||
| idxBuilder: Expr => Expr |
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.
Is there a reason why this needs to be a lambda and not just a simple Expr that evaluates to a single non-negative integer?
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.
I think it needs to be lambda otherwise we end up with duplicating the index computation in both branches because we only get correct tupleRef inside each branch. And also negative indices require array.len(tupleRef)
hkmc2/shared/src/main/scala/hkmc2/codegen/wasm/text/WatBuilder.scala
Outdated
Show resolved
Hide resolved
| val tupleIsMutable = ref.test(tupleExpr, RefType(mutArrayType, nullable = true)) | ||
| val mutableBranch = | ||
| val tupleRef = ref.cast(tupleExpr, RefType(mutArrayType, nullable = false)) | ||
| array.get(mutArrayType, tupleRef, idxBuilder(tupleRef), elemType) | ||
| val immutableBranch = | ||
| val tupleRef = ref.cast(tupleExpr, RefType(immArrayType, nullable = false)) | ||
| array.get(immArrayType, tupleRef, idxBuilder(tupleRef), elemType) |
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.
Question: If tupleExpr evaluates to an expression with possible side effects (e.g. a function call), would this repeated use of tupleExpr here cause the expression to be emitted 3 times and invoked twice during execution?
For example, in makePair(1, 2).[1], would makePair(1, 2) be evaluated twice?
If so, it may be worth it to generate a temporary local to store the result of tupleExpr, roughly like:
(if
(local.tee $_tmp
(ref.test (...))
(then
(array.get
(ref.cast
(local.get $_tmp)))(local.tee sets the result of the child expression into the local, and returns the same value.)
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.
ok I added local.tee but it required extra handling for temp locals
| condition = i32.lt_s(idxI32, i32.const(0)), | ||
| ifTrue = i32.add(idxI32, array.len(tupleRef)), | ||
| ifFalse = S(idxI32), |
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 idxI32 expression is emitted three times and will be executed twice during runtime, similar to this comment.
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.
ok
No description provided.