pulley: Implement vector sqmul_round_sat #9911
Conversation
eagr
requested review from
cfallin and
fitzgen
and removed request for
a team
github-actions
bot
added
cranelift
Subscribe to Label Actioncc @fitzgen
This issue or pull request has been labeled: "cranelift", "cranelift:meta", "pulley"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
pulley/src/interp.rs
Outdated
| const MIN: i32 = i16::MIN as i32; | ||
| const MAX: i32 = i16::MAX as i32; | ||
| for (a, b) in a.iter_mut().zip(b) { | ||
| let r = i32::from(*a) * i32::from(b) + (1 << 14) >> 15; |
There was a problem hiding this comment.
I'm a bit surprised by the lack of parentheses around this, I would have expected:
let r = (i32::from(*a) * i32::from(b) + (1 << 14)) >> 15;as otherwise the final term (1 << 14) >> 15 looks like it currently evaluates to 0.
Tests, however, are passing with this PR which means that either the precedence rules of parsing in Rust are different from what I expect or that the tests don't actually cover the case which needs the parentheses to be right here. Would you be up for debugging this a bit to figure out what's going on test-wise?
There was a problem hiding this comment.
You must have always been in the good habit of applying parentheses when in doubt so you don't have to remember the precedence rules :)
I think this is equivalent to what you expect. Let me just add those parentheses and you'd see the same result. The code reads better that way anyways. And btw the tests are looking good to me.
There was a problem hiding this comment.
Oh nice, thanks for checking!
2 participants
part of #9783