Skip to content
This repository has been archived by the owner on Jul 12, 2024. It is now read-only.

Implement monomorphic Array #16

Merged
merged 3 commits into from
Mar 19, 2024
Merged

Implement monomorphic Array #16

merged 3 commits into from
Mar 19, 2024

Conversation

tanishiking
Copy link
Owner

@tanishiking tanishiking commented Mar 14, 2024

Better to work based on #15 for js.Iterable...

Implement fundamental monomorphic array

@sjrd
Copy link
Collaborator

sjrd commented Mar 14, 2024

I don't think #15 enters into play here. I hope we can keep arrays entirely inside Wasm.

@tanishiking tanishiking force-pushed the array branch 4 times, most recently from 07ac844 to 9235493 Compare March 14, 2024 14:26
@tanishiking tanishiking changed the title WIP: Implement Array Implement monomorphic Array Mar 14, 2024
@tanishiking tanishiking marked this pull request as ready for review March 14, 2024 14:38
Copy link
Collaborator

@sjrd sjrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall. Some minor suggestions.

Comment on lines 163 to 168
// case IRTrees.IsInstanceOf(pos) =>
// case IRTrees.JSLinkingInfo(pos) =>
// case IRTrees.Select(tpe) =>
// case IRTrees.Return(pos) =>
// case IRTrees.While(pos) =>
// case IRTrees.LoadJSConstructor(pos) =>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These shouldn't reappear. We did implement them.

Comment on lines 151 to 153
case t: IRTrees.Throw =>
instrs += UNREACHABLE // Implement Exception Handling (Throwable doesn't compile yet)
IRTypes.NothingType
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undo the case t: IRTrees.Throw => since #17 used null.toString() to achieve the same thing?

Or put it in genThrow(t) to keep the shape of this pattern match?

)

def genWithDimensions(typeRef: IRTypes.ArrayTypeRef, lengths: List[IRTrees.Tree]): Unit = {
val elemTy = extractArrayElemType(t.typeRef)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is inside the recursive genWithDimensions but it refers to t which is captured from the enclosing genNewArray. That's suspicious. Was it meant to be the following instead?

Suggested change
val elemTy = extractArrayElemType(t.typeRef)
val elemTy = extractArrayElemType(typeRef)

Comment on lines 1620 to 1686
val irElemTy = extractArrayElemType(t.typeRef)
val wasmElemTy = TypeTransformer.transformType(irElemTy)(ctx)
val arrTyName = Names.WasmTypeName.WasmArrayTypeName(t.tpe)
ctx.addArrayType(
WasmArrayType(
arrTyName,
WasmStructField(Names.WasmFieldName.arrayField, wasmElemTy, isMutable = true)
)
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 9 lines are copy-pasted between here (genArrayValue) and genNewArray. Consider factoring them directly inside a method of ctx, for example

def getArrayTypeName(typeRef: IRTypes.ArrayTypeRef): WasmArrayTypeName = ...

Comment on lines 1583 to 1644
genWithDimensions(
IRTypes.ArrayTypeRef(typeRef.base, typeRef.dimensions - 1),
lengths.tail
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, this will create a unique nested array and use it for all the elements of the outer array. In other words, it will compile

new Array[Array[Int]](2, 2)

into something equivalent to

val inner = new Array[Int](2)
val outer = Array(inner, inner)

But it is supposed to create a different instance of the inner array for each slot of the outer array.

I don't think it is possible to do this without a generic recursive helper, which we don't have enough infrastructure for at the moment.

I suggest focusing on the case where lengths.size == 1 for now, and leave a ??? for this case. It is rather rare, so we should be able to make a lot of code work without that support.

Comment on lines 1637 to 1698
typeRef.base match {
case IRTypes.ClassRef(className) => ClassType(className)
case IRTypes.PrimRef(tpe) => tpe
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
typeRef.base match {
case IRTypes.ClassRef(className) => ClassType(className)
case IRTypes.PrimRef(tpe) => tpe
}
ctx.inferTypeFromTypeRef(typeRef.base)

Copy link
Collaborator

@sjrd sjrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, excellent :)

@sjrd sjrd merged commit 4643e4b into main Mar 19, 2024
1 check passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants