Use bitwise operations to get the absolute values of integers #113679
Conversation
Helps the JIT resolving it as CMOV
|
I'd suggest to hold on before more conclusion is made by area owners of System.Numerics and JIT in the original issue. For smaller types (short and sbyte), the performance characteristics can be drastically different because they must be widen to 32bit when doing arithmetic or bitwise operations. |
|
Your benchmarks in #113651 measure the cost of branch misprediction primarily since your picked input data makes the branch completely unpredictable. This is worst case scenario for the branch based version. In practice branches are typically easy to predict. What do your results look like if the branch is predictable? (e.g. make all array elements positive or negative, or make it a more predictable mix). I think we would rather want to write this in a way that makes it amenable to if-conversion (such that |
Although I'm not sure if the branch prediction is that good in regular cases (that might be), I fully agree with your (and @huoyaoyuan ) suggestion to address this the JIT should be able to use |
| } | ||
| return value; | ||
|
|
||
| return unchecked((short)((value + (value >>= 15)) ^ value)); |
There was a problem hiding this comment.
This should really be broken apart, to make it clearer and potentially have an explanatory comment on how it works (sign is 0 for positive and 1 for negative, thus it computes the original value for positive and ~value + 1 for negative, which is to say -value):
int sign = value >> ((sizeof(T) * 8) - 1);
return unchecked((T)((value + sign) ^ sign));However, since we need a branch regardless we're basically looking at 2 branches vs 3 instructions + 1 branch
I agree with @jakobbotsch that it would be better to improve the conditional select logic first and foremost. Reducing it down to one branch in typical cases would also be goodness, but ideally we could have that participate with PGO/DPGO so that we don't pessimize it unnecessarily.
Ideally we could generate something like:
mov eax, ecx
neg ecx
cmovns eax, ecx
js THROW
retWhich would require some minor optimization work and flag reuse on top of (avoiding the redundant test reg, reg its currently generating):
int result = x;
if (result < 0)
{
result = -x;
}
if (result < 0)
{
Throw();
}
return result;There was a problem hiding this comment.
@tannergooding Should be the reasoning behind such changes be documented in the code? In this case all the logic written down is to help the JIT doing JIT things,
There was a problem hiding this comment.
When the code isn't "obvious" or the reasoning for a change isn't necessarily clear, yes. This helps future readers understand and can help avoid accidental regressions
Helps the JIT resolving it by applying CMOV.
Closes #113651