-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[RyuJit Wasm] fix off by one error for br_table #123565
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: main
Are you sure you want to change the base?
Conversation
The count of switch targets does not include the default.
|
@dotnet/jit PTAL We can now run examples with switches, like so: static int sw(int n)
{
switch (n)
{
case 1: case 3: return 10;
case 2: case 4: return 20;
case 5: case 7: return 30;
case 6: case 8: return 40;
default: return 0;
}
}via @kg's harness |
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.
Pull request overview
This PR fixes an off-by-one error in the RyuJIT WASM code generation for switch statements using the br_table instruction.
Changes:
- Fixed the count parameter passed to
emitIns_IforINS_br_tableto correctly exclude the default case from the count
|
|
||
| GetEmitter()->emitIns_I(INS_br_table, EA_4BYTE, caseCount); | ||
| GetEmitter()->emitIns_I(INS_br_table, EA_4BYTE, caseCount - 1); | ||
|
|
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.
Can you potentially add a comment here explaining that this loop will handle emitting all the targets, including the default, even though in the above count we excluded it because of Wasm's convention? We could also consider pulling emitting the default out of the loop just to be extra clear here, though I know we also have the assert above to ensure we do have a default case.
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.
Yes. Take another look.
The count of switch targets does not include the default.