Skip to content

Conversation

@tlively
Copy link
Member

@tlively tlively commented Nov 10, 2023

Also mark array.new_elem as unimplemented as a drive-by; it previously had an
incorrect implementation.

@tlively
Copy link
Member Author

tlively commented Nov 10, 2023

;; CHECK-NEXT: (local $0 (ref null $void))
;; CHECK-NEXT: (local $1 (ref null $ret2))
;; CHECK-NEXT: (local $2 (ref null $many))
;; CHECK-NEXT: (local $scratch (i32 i32))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why were these scratch values created?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the call to many returns a tuple, which is then accessed by multiple different drop instructions. Since we have an expression tree, each value can only be consumed by one expression (its parent) normally. Since tuples need to be accessible by multiple expressions, they need to be stashed in locals first. The scratch locals are the locals IRBuilder introduces to hold these tuples.

auto data = dataidx(ctx);
CHECK_ERR(data);
return ctx.makeArrayNewElem(pos, *type, *data);
return ctx.in.err("unimplemented instruction");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this now unimplemented?

Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed that it incorrectly used dataidx above, when it should have used elemidx, which doesn't exist yet. I removed the incorrect implementation with the standard "unimplemented" error.

@tlively
Copy link
Member Author

tlively commented Nov 15, 2023

Merge activity

  • Nov 14, 7:40 PM: @tlively started a stack merge that includes this pull request via Graphite.
  • Nov 14, 7:48 PM: Graphite rebased this pull request as part of a merge.
  • Nov 14, 7:49 PM: @tlively merged this pull request with Graphite.

@tlively tlively force-pushed the parser-array-new-fixed branch from c557024 to d8f004f Compare November 15, 2023 00:47
Base automatically changed from parser-array-new-fixed to main November 15, 2023 00:48
Also mark array.new_elem as unimplemented as a drive-by; it previously had an
incorrect implementation.
@tlively tlively merged commit 001be44 into main Nov 15, 2023
@tlively tlively deleted the parser-call-ref branch November 15, 2023 00:49
radekdoulik pushed a commit to dotnet/binaryen that referenced this pull request Jul 12, 2024
Also mark array.new_elem as unimplemented as a drive-by; it previously had an
incorrect implementation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants