-
Notifications
You must be signed in to change notification settings - Fork 675
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
Add support for atomic.fence from the threads proposal #1231
Conversation
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.
lgtm, with a couple of comments
@@ -134,7 +135,7 @@ WABT_TOKEN(Throw, "throw") | |||
WABT_TOKEN(Try, "try") | |||
WABT_TOKEN(Unary, "UNARY") | |||
WABT_TOKEN(Unreachable, "unreachable") | |||
WABT_TOKEN_FIRST(Opcode, AtomicLoad) |
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.
Was AtomicLoad not supposed to be here at all?
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.
Mmm, I think this was a bad change that snuck in. I will make sure it's not there in the revised patch and regenerate the lexer. EDIT: Actually this patch is the right thing. WABT_TOKEN_FIRST specifies the first Opcode token. It was AtomicLoad, but I added AtomicFence before it, so here I replace the operand.
return Result::Error; | ||
} | ||
return Result::Ok; | ||
} |
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 the type checker the right place for this? i.e. is this a more general validation thing than a type checking thing?
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.
Hooo, finally picking up this patch again. Rebasing and re-uploading, but yes you are right -- will include this check in the validator rather than the type checker.
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.
Addressing comments before rebase
return Result::Error; | ||
} | ||
return Result::Ok; | ||
} |
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.
Hooo, finally picking up this patch again. Rebasing and re-uploading, but yes you are right -- will include this check in the validator rather than the type checker.
@@ -134,7 +135,7 @@ WABT_TOKEN(Throw, "throw") | |||
WABT_TOKEN(Try, "try") | |||
WABT_TOKEN(Unary, "UNARY") | |||
WABT_TOKEN(Unreachable, "unreachable") | |||
WABT_TOKEN_FIRST(Opcode, AtomicLoad) |
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.
Mmm, I think this was a bad change that snuck in. I will make sure it's not there in the revised patch and regenerate the lexer. EDIT: Actually this patch is the right thing. WABT_TOKEN_FIRST specifies the first Opcode token. It was AtomicLoad, but I added AtomicFence before it, so here I replace the operand.
See WebAssembly/threads#141 for the binary encoding. This patch does add a field to AtomicFenceExpr for the consistency model, though without a type for the time being.
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 doing this! lgtm
See WebAssembly/threads#141 for the binary encoding. This patch does add a field to
AtomicFenceExpr
for the consistency model, though without a type for the time being.