Open Bug 1301379 Opened 5 years ago Updated 5 years ago
Hoist bounds checks for strided array indices
I will try to find proper time to review all the consequences of adding Rsh as part of LinearSum, as Rsh is not a linear operation, it wraps-around its inputs to the int32 range. This technically imply that we would have to be careful that bounds of the operands are always within the wrap-around range, to ensure that such binary operations are no-ops.
Can you take another look at the shift right zero utility function and/or the whole patch?
(In reply to Nicolas B. Pierron [:nbp] from comment #1) > I will try to find proper time to review all the consequences of adding Rsh > as part of LinearSum, as Rsh is not a linear operation, it wraps-around its > inputs to the int32 range. > > This technically imply that we would have to be careful that bounds of the > operands are always within the wrap-around range, to ensure that such binary > operations are no-ops. Sorry for the delay, but I think that history proved me right to have such concerns. Have a look at Bug 1319242, and rebase this patch accordingly.
Comment on attachment 8789362 [details] [diff] [review] hoist-bounds-checks-for-constant-step-and-shift-right-zero.patch Review of attachment 8789362 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonAnalysis.cpp @@ +3046,5 @@ > return SimpleLinearSum(nullptr, ins->toConstant()->toInt32()); > > + // Shift right with zero does not change the sum. > + if (IsRightShiftZero(ins)) > + return ExtractLinearSum(ins->getOperand(0)); I disargree a right shift by zero does change the sum as this is a bitwise instruction, as this means that operations before it are in the Modulo 2³² space, while its uses are in an Infinite integer space. Have a look at the test case from attachment 8813303 [details] [diff] [review]. When the truncated input overflow in the Modulo 2³² space, we wrap it around, and moving it out-side the Modulo 2³² space changes the result. This should not affect asm.js-like code as all operations are truncated by default, but this could cause differential behaviour on ordinary JS code.
You need to log in before you can comment on or make changes to this bug.