| Message ID | 1435259818-6864-1-git-send-email-aurelien@aurel32.net |
|---|---|
| State | New |
| Headers | show |
On 06/25/2015 08:16 PM, Aurelien Jarno wrote: > for (shift = 4; (shift < 64) && bin; shift += 4) { > - int current_number = bin % 10; > - > - dec |= (current_number) << shift; > + dec |= (bin % 10) << shift; > bin /= 10; > } You've changed from 32-bit division to 64-bit division just to solve a problem with the shift. Better to just change the type of current_number there. r~
On 2015-06-29 10:28, Richard Henderson wrote: > On 06/25/2015 08:16 PM, Aurelien Jarno wrote: > > for (shift = 4; (shift < 64) && bin; shift += 4) { > >- int current_number = bin % 10; > >- > >- dec |= (current_number) << shift; > >+ dec |= (bin % 10) << shift; > > bin /= 10; > > } > > You've changed from 32-bit division to 64-bit division just to solve a > problem with the shift. Better to just change the type of current_number > there. Changing the type of current_number instead of bin would indeed solve the shift issue, but not the -2^31 case. As we take the absolute value, we need a 64-bit variable to hold the corresponding 2^31 value.
On 06/29/2015 09:24 PM, Aurelien Jarno wrote: > On 2015-06-29 10:28, Richard Henderson wrote: >> On 06/25/2015 08:16 PM, Aurelien Jarno wrote: >>> for (shift = 4; (shift < 64) && bin; shift += 4) { >>> - int current_number = bin % 10; >>> - >>> - dec |= (current_number) << shift; >>> + dec |= (bin % 10) << shift; >>> bin /= 10; >>> } >> >> You've changed from 32-bit division to 64-bit division just to solve a >> problem with the shift. Better to just change the type of current_number >> there. > > Changing the type of current_number instead of bin would indeed solve > the shift issue, but not the -2^31 case. As we take the absolute value, > we need a 64-bit variable to hold the corresponding 2^31 value. > Ah, true enough. I suppose adding a 32-bit unsigned variable with which to do the division is more trouble than it's worth. Reviewed-by: Richard Henderson <rth@twiddle.net> r~
On 30.06.15 07:53, Richard Henderson wrote: > On 06/29/2015 09:24 PM, Aurelien Jarno wrote: >> On 2015-06-29 10:28, Richard Henderson wrote: >>> On 06/25/2015 08:16 PM, Aurelien Jarno wrote: >>>> for (shift = 4; (shift < 64) && bin; shift += 4) { >>>> - int current_number = bin % 10; >>>> - >>>> - dec |= (current_number) << shift; >>>> + dec |= (bin % 10) << shift; >>>> bin /= 10; >>>> } >>> >>> You've changed from 32-bit division to 64-bit division just to solve a >>> problem with the shift. Better to just change the type of >>> current_number >>> there. >> >> Changing the type of current_number instead of bin would indeed solve >> the shift issue, but not the -2^31 case. As we take the absolute value, >> we need a 64-bit variable to hold the corresponding 2^31 value. >> > > Ah, true enough. I suppose adding a 32-bit unsigned variable with which > to do the division is more trouble than it's worth. > > Reviewed-by: Richard Henderson <rth@twiddle.net> Thanks, applied to s390-next. Alex
diff --git a/target-s390x/int_helper.c b/target-s390x/int_helper.c index 2c2b3f6..a46c736 100644 --- a/target-s390x/int_helper.c +++ b/target-s390x/int_helper.c @@ -121,11 +121,12 @@ uint64_t HELPER(clz)(uint64_t v) return clz64(v); } -uint64_t HELPER(cvd)(int32_t bin) +uint64_t HELPER(cvd)(int32_t reg) { /* positive 0 */ uint64_t dec = 0x0c; - int shift = 4; + int64_t bin = reg; + int shift; if (bin < 0) { bin = -bin; @@ -133,9 +134,7 @@ uint64_t HELPER(cvd)(int32_t bin) } for (shift = 4; (shift < 64) && bin; shift += 4) { - int current_number = bin % 10; - - dec |= (current_number) << shift; + dec |= (bin % 10) << shift; bin /= 10; }
current_number being shift left by more than 32 bits, we can't use a simple int. Similarly use an int64_t type for the input binary value, to not get the -2^31 case wrong. Finally don't initialize shift to 4, it's already done in the for loop. Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> Cc: Alexander Graf <agraf@suse.de> Cc: Richard Henderson <rth@twiddle.net> --- target-s390x/int_helper.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)