| Message ID | 1425720834-5811-1-git-send-email-sw@weilnetz.de |
|---|---|
| State | Superseded |
| Headers | show |
Hi Stefan, On 03/07/2015 09:33 AM, Stefan Weil wrote: > clang report: > > target-tricore/op_helper.c:1247:24: warning: > taking the absolute value of unsigned type 'uint32_t' (aka 'unsigned int') > has no effect [-Wabsolute-value] > target-tricore/op_helper.c:1248:25: warning: > taking the absolute value of unsigned type 'uint32_t' (aka 'unsigned int') > has no effect [-Wabsolute-value] > target-tricore/op_helper.c:1249:19: warning: > taking the absolute value of unsigned type 'uint32_t' (aka 'unsigned int') > has no effect [-Wabsolute-value] > target-tricore/op_helper.c:1297:24: warning: > taking the absolute value of unsigned type 'uint32_t' (aka 'unsigned int') > has no effect [-Wabsolute-value] > target-tricore/op_helper.c:1298:25: warning: > taking the absolute value of unsigned type 'uint32_t' (aka 'unsigned int') > has no effect [-Wabsolute-value] > target-tricore/op_helper.c:1299:19: warning: > taking the absolute value of unsigned type 'uint32_t' (aka 'unsigned int') > has no effect [-Wabsolute-value] > > Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de> > Signed-off-by: Stefan Weil <sw@weilnetz.de> > --- Good catch. I'm really confused, that this was not breaking my test suit. Your fix looks good to me at first sight, but I'll look into the problem. Cheers, Bastian
On 7 March 2015 at 18:33, Stefan Weil <sw@weilnetz.de> wrote: > diff --git a/target-tricore/op_helper.c b/target-tricore/op_helper.c > index ed26b30..52ad80b 100644 > --- a/target-tricore/op_helper.c > +++ b/target-tricore/op_helper.c > @@ -1244,9 +1244,9 @@ uint64_t helper_dvinit_b_13(CPUTriCoreState *env, uint32_t r1, uint32_t r2) > quotient_sign = 1; > } > > - abs_sig_dividend = abs(r1) >> 7; > - abs_base_dividend = abs(r1) & 0x7f; > - abs_divisor = abs(r1); > + abs_sig_dividend = abs((int32_t)r1) >> 7; > + abs_base_dividend = abs((int32_t)r1) & 0x7f; > + abs_divisor = abs((int32_t)r1); Also, surely some of these should be using r2, not r1? At the moment we seem to use r1 for both dividend and divisor, which does not look right at all... -- PMM
On 03/07/2015 12:34 PM, Peter Maydell wrote: > On 7 March 2015 at 18:33, Stefan Weil <sw@weilnetz.de> wrote: >> diff --git a/target-tricore/op_helper.c b/target-tricore/op_helper.c >> index ed26b30..52ad80b 100644 >> --- a/target-tricore/op_helper.c >> +++ b/target-tricore/op_helper.c >> @@ -1244,9 +1244,9 @@ uint64_t helper_dvinit_b_13(CPUTriCoreState *env, uint32_t r1, uint32_t r2) >> quotient_sign = 1; >> } >> >> - abs_sig_dividend = abs(r1) >> 7; >> - abs_base_dividend = abs(r1) & 0x7f; >> - abs_divisor = abs(r1); >> + abs_sig_dividend = abs((int32_t)r1) >> 7; >> + abs_base_dividend = abs((int32_t)r1) & 0x7f; >> + abs_divisor = abs((int32_t)r1); > Also, surely some of these should be using r2, not r1? > At the moment we seem to use r1 for both dividend and > divisor, which does not look right at all... > > -- PMM Thats correct, it should be: abs_divisor = abs((int32_t)r2); My test suit did not get this, because I was only testing with a ISA version 1.3.1 cpu and this is 1.3 exclusive. Thanks for finding those. Cheers, Bastian
diff --git a/target-tricore/op_helper.c b/target-tricore/op_helper.c index ed26b30..52ad80b 100644 --- a/target-tricore/op_helper.c +++ b/target-tricore/op_helper.c @@ -1244,9 +1244,9 @@ uint64_t helper_dvinit_b_13(CPUTriCoreState *env, uint32_t r1, uint32_t r2) quotient_sign = 1; } - abs_sig_dividend = abs(r1) >> 7; - abs_base_dividend = abs(r1) & 0x7f; - abs_divisor = abs(r1); + abs_sig_dividend = abs((int32_t)r1) >> 7; + abs_base_dividend = abs((int32_t)r1) & 0x7f; + abs_divisor = abs((int32_t)r1); /* calc overflow */ env->PSW_USB_V = 0; if ((quotient_sign) && (abs_divisor)) { @@ -1294,9 +1294,9 @@ uint64_t helper_dvinit_h_13(CPUTriCoreState *env, uint32_t r1, uint32_t r2) quotient_sign = 1; } - abs_sig_dividend = abs(r1) >> 7; - abs_base_dividend = abs(r1) & 0x7f; - abs_divisor = abs(r1); + abs_sig_dividend = abs((int32_t)r1) >> 7; + abs_base_dividend = abs((int32_t)r1) & 0x7f; + abs_divisor = abs((int32_t)r1); /* calc overflow */ env->PSW_USB_V = 0; if ((quotient_sign) && (abs_divisor)) {
clang report: target-tricore/op_helper.c:1247:24: warning: taking the absolute value of unsigned type 'uint32_t' (aka 'unsigned int') has no effect [-Wabsolute-value] target-tricore/op_helper.c:1248:25: warning: taking the absolute value of unsigned type 'uint32_t' (aka 'unsigned int') has no effect [-Wabsolute-value] target-tricore/op_helper.c:1249:19: warning: taking the absolute value of unsigned type 'uint32_t' (aka 'unsigned int') has no effect [-Wabsolute-value] target-tricore/op_helper.c:1297:24: warning: taking the absolute value of unsigned type 'uint32_t' (aka 'unsigned int') has no effect [-Wabsolute-value] target-tricore/op_helper.c:1298:25: warning: taking the absolute value of unsigned type 'uint32_t' (aka 'unsigned int') has no effect [-Wabsolute-value] target-tricore/op_helper.c:1299:19: warning: taking the absolute value of unsigned type 'uint32_t' (aka 'unsigned int') has no effect [-Wabsolute-value] Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de> Signed-off-by: Stefan Weil <sw@weilnetz.de> --- I'm not sure whether this is the correct fix, so please review carefully. Thanks, Stefan target-tricore/op_helper.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)