| Message ID | 1446473134-4330-1-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 2 November 2015 at 14:05, Paolo Bonzini <pbonzini@redhat.com> wrote: > This is reported by Coverity. The algorithm description at > ftp://ftp.icm.edu.pl/packages/ggi/doc/hw/sparc/Sparc.pdf suggests > that the 32-bit parts of rs2, after the left shift, is treated > as a 64-bit integer. Bits 32 and above are used to do the > saturating truncation. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > target-sparc/vis_helper.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target-sparc/vis_helper.c b/target-sparc/vis_helper.c > index 383cc8b..45fc7db 100644 > --- a/target-sparc/vis_helper.c > +++ b/target-sparc/vis_helper.c > @@ -447,7 +447,7 @@ uint32_t helper_fpackfix(uint64_t gsr, uint64_t rs2) > for (word = 0; word < 2; word++) { > uint32_t val; > int32_t src = rs2 >> (word * 32); > - int64_t scaled = src << scale; > + int64_t scaled = (int64_t)src << scale; > int64_t from_fixed = scaled >> 16; This will now shift left into the sign bit of a signed integer, which is undefined behaviour. thanks -- PMM
On 02/11/2015 15:09, Peter Maydell wrote: >> > diff --git a/target-sparc/vis_helper.c b/target-sparc/vis_helper.c >> > index 383cc8b..45fc7db 100644 >> > --- a/target-sparc/vis_helper.c >> > +++ b/target-sparc/vis_helper.c >> > @@ -447,7 +447,7 @@ uint32_t helper_fpackfix(uint64_t gsr, uint64_t rs2) >> > for (word = 0; word < 2; word++) { >> > uint32_t val; >> > int32_t src = rs2 >> (word * 32); >> > - int64_t scaled = src << scale; >> > + int64_t scaled = (int64_t)src << scale; >> > int64_t from_fixed = scaled >> 16; > This will now shift left into the sign bit of a signed integer, > which is undefined behaviour. Why "now"? It would have done the same before. Paolo
On 2 November 2015 at 14:48, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 02/11/2015 15:09, Peter Maydell wrote: >>> > diff --git a/target-sparc/vis_helper.c b/target-sparc/vis_helper.c >>> > index 383cc8b..45fc7db 100644 >>> > --- a/target-sparc/vis_helper.c >>> > +++ b/target-sparc/vis_helper.c >>> > @@ -447,7 +447,7 @@ uint32_t helper_fpackfix(uint64_t gsr, uint64_t rs2) >>> > for (word = 0; word < 2; word++) { >>> > uint32_t val; >>> > int32_t src = rs2 >> (word * 32); >>> > - int64_t scaled = src << scale; >>> > + int64_t scaled = (int64_t)src << scale; >>> > int64_t from_fixed = scaled >> 16; >> This will now shift left into the sign bit of a signed integer, >> which is undefined behaviour. > > Why "now"? It would have done the same before. True, but I was reviewing the new code rather than the code you were taking away :-) Incidentally, that manual says the fpackfix and fpack32 insns use a 4 bit GSR.scale_factor value, but our code is masking by 0x1f in helper_fpack32 and helper_fpackfix. Which is right? thanks -- PMM
On 02/11/2015 16:13, Peter Maydell wrote: > On 2 November 2015 at 14:48, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> >> On 02/11/2015 15:09, Peter Maydell wrote: >>>>> diff --git a/target-sparc/vis_helper.c b/target-sparc/vis_helper.c >>>>> index 383cc8b..45fc7db 100644 >>>>> --- a/target-sparc/vis_helper.c >>>>> +++ b/target-sparc/vis_helper.c >>>>> @@ -447,7 +447,7 @@ uint32_t helper_fpackfix(uint64_t gsr, uint64_t rs2) >>>>> for (word = 0; word < 2; word++) { >>>>> uint32_t val; >>>>> int32_t src = rs2 >> (word * 32); >>>>> - int64_t scaled = src << scale; >>>>> + int64_t scaled = (int64_t)src << scale; >>>>> int64_t from_fixed = scaled >> 16; >>> This will now shift left into the sign bit of a signed integer, >>> which is undefined behaviour. >> >> Why "now"? It would have done the same before. > > True, but I was reviewing the new code rather than the > code you were taking away :-) > > Incidentally, that manual says the fpackfix and fpack32 insns > use a 4 bit GSR.scale_factor value, but our code is masking > by 0x1f in helper_fpack32 and helper_fpackfix. Which is right? I don't know... That manual even says that GSR has no bit defined above bit 6 (where scale_factor is 3 to 6). It's possible that a newer processor defines a 5-bit scale factor; I honestly have no idea. Paolo
On 11/02/2015 04:13 PM, Peter Maydell wrote: > On 2 November 2015 at 14:48, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> >> On 02/11/2015 15:09, Peter Maydell wrote: >>>>> diff --git a/target-sparc/vis_helper.c b/target-sparc/vis_helper.c >>>>> index 383cc8b..45fc7db 100644 >>>>> --- a/target-sparc/vis_helper.c >>>>> +++ b/target-sparc/vis_helper.c >>>>> @@ -447,7 +447,7 @@ uint32_t helper_fpackfix(uint64_t gsr, uint64_t rs2) >>>>> for (word = 0; word < 2; word++) { >>>>> uint32_t val; >>>>> int32_t src = rs2 >> (word * 32); >>>>> - int64_t scaled = src << scale; >>>>> + int64_t scaled = (int64_t)src << scale; >>>>> int64_t from_fixed = scaled >> 16; >>> This will now shift left into the sign bit of a signed integer, >>> which is undefined behaviour. >> >> Why "now"? It would have done the same before. > > True, but I was reviewing the new code rather than the > code you were taking away :-) > > Incidentally, that manual says the fpackfix and fpack32 insns > use a 4 bit GSR.scale_factor value, but our code is masking > by 0x1f in helper_fpack32 and helper_fpackfix. Which is right? The 2011 manual has 5 bits for fpack32 and fpackfix; fpack16 uses only 4 bits. I do think we'd be better served by casting to uint64_t on that line. Note that fpackfix requires the same correction. And it wouldn't hurt to cast to uint32_t in fpack16, lest we anger the self-same shifting gods. r~
On 04/11/2015 11:12, Richard Henderson wrote: > On 11/02/2015 04:13 PM, Peter Maydell wrote: >> On 2 November 2015 at 14:48, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> >>> >>> On 02/11/2015 15:09, Peter Maydell wrote: >>>>>> diff --git a/target-sparc/vis_helper.c b/target-sparc/vis_helper.c >>>>>> index 383cc8b..45fc7db 100644 >>>>>> --- a/target-sparc/vis_helper.c >>>>>> +++ b/target-sparc/vis_helper.c >>>>>> @@ -447,7 +447,7 @@ uint32_t helper_fpackfix(uint64_t gsr, >>>>>> uint64_t rs2) >>>>>> for (word = 0; word < 2; word++) { >>>>>> uint32_t val; >>>>>> int32_t src = rs2 >> (word * 32); >>>>>> - int64_t scaled = src << scale; >>>>>> + int64_t scaled = (int64_t)src << scale; >>>>>> int64_t from_fixed = scaled >> 16; >>>> This will now shift left into the sign bit of a signed integer, >>>> which is undefined behaviour. >>> >>> Why "now"? It would have done the same before. >> >> True, but I was reviewing the new code rather than the >> code you were taking away :-) >> >> Incidentally, that manual says the fpackfix and fpack32 insns >> use a 4 bit GSR.scale_factor value, but our code is masking >> by 0x1f in helper_fpack32 and helper_fpackfix. Which is right? > > The 2011 manual has 5 bits for fpack32 and fpackfix; fpack16 uses only 4 > bits. > > I do think we'd be better served by casting to uint64_t on that line. > Note that fpackfix requires the same correction. And it wouldn't hurt > to cast to uint32_t in fpack16, lest we anger the self-same shifting gods. Hmmm.. say src = -0x80000000, scale = 1; scaled = (uint64_t)-0x8000000 << 1 = 0xffffffff00000000 from_fixed = 0xffffffff00000000 >> 16 = 0x0000ffffffff0000 Now from_fixed is positive and you get 32767 instead of -32768. In other words, we would have to cast to uint64_t on the scaled assignment, and back to int64_t on the from_fixed assignment. I must be misunderstanding your suggestion. Paolo
On 11/04/2015 11:45 AM, Paolo Bonzini wrote: >>>>>>> int32_t src = rs2 >> (word * 32); >>>>>>> - int64_t scaled = src << scale; >>>>>>> + int64_t scaled = (int64_t)src << scale; >>>>>>> int64_t from_fixed = scaled >> 16; ... >> >> I do think we'd be better served by casting to uint64_t on that line. >> Note that fpackfix requires the same correction. And it wouldn't hurt >> to cast to uint32_t in fpack16, lest we anger the self-same shifting gods. > > Hmmm.. say src = -0x80000000, scale = 1; > > scaled = (uint64_t)-0x8000000 << 1 = 0xffffffff00000000 > from_fixed = 0xffffffff00000000 >> 16 = 0x0000ffffffff0000 > > Now from_fixed is positive and you get 32767 instead of -32768. In > other words, we would have to cast to uint64_t on the scaled assignment, > and back to int64_t on the from_fixed assignment. I must be > misunderstanding your suggestion. int64_t scaled = (uint64_t)src << scale; I.e. one explicit conversion and one implicit conversion. r~
On 04/11/2015 12:05, Richard Henderson wrote: > On 11/04/2015 11:45 AM, Paolo Bonzini wrote: >>>>>>>> int32_t src = rs2 >> (word * 32); >>>>>>>> - int64_t scaled = src << scale; >>>>>>>> + int64_t scaled = (int64_t)src << scale; >>>>>>>> int64_t from_fixed = scaled >> 16; > ... >>> >>> I do think we'd be better served by casting to uint64_t on that line. >>> Note that fpackfix requires the same correction. And it wouldn't hurt >>> to cast to uint32_t in fpack16, lest we anger the self-same shifting >>> gods. >> >> Hmmm.. say src = -0x80000000, scale = 1; >> >> scaled = (uint64_t)-0x8000000 << 1 = 0xffffffff00000000 >> from_fixed = 0xffffffff00000000 >> 16 = 0x0000ffffffff0000 >> >> Now from_fixed is positive and you get 32767 instead of -32768. In >> other words, we would have to cast to uint64_t on the scaled assignment, >> and back to int64_t on the from_fixed assignment. I must be >> misunderstanding your suggestion. > > int64_t scaled = (uint64_t)src << scale; > > I.e. one explicit conversion and one implicit conversion. That does the job, but it also does look like a typo... Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 04/11/2015 12:05, Richard Henderson wrote: >> On 11/04/2015 11:45 AM, Paolo Bonzini wrote: >>>>>>>>> int32_t src = rs2 >> (word * 32); >>>>>>>>> - int64_t scaled = src << scale; >>>>>>>>> + int64_t scaled = (int64_t)src << scale; >>>>>>>>> int64_t from_fixed = scaled >> 16; >> ... >>>> >>>> I do think we'd be better served by casting to uint64_t on that line. >>>> Note that fpackfix requires the same correction. And it wouldn't hurt >>>> to cast to uint32_t in fpack16, lest we anger the self-same shifting >>>> gods. >>> >>> Hmmm.. say src = -0x80000000, scale = 1; >>> >>> scaled = (uint64_t)-0x8000000 << 1 = 0xffffffff00000000 >>> from_fixed = 0xffffffff00000000 >> 16 = 0x0000ffffffff0000 >>> >>> Now from_fixed is positive and you get 32767 instead of -32768. In >>> other words, we would have to cast to uint64_t on the scaled assignment, >>> and back to int64_t on the from_fixed assignment. I must be >>> misunderstanding your suggestion. >> >> int64_t scaled = (uint64_t)src << scale; >> >> I.e. one explicit conversion and one implicit conversion. > > That does the job, but it also does look like a typo... Make the implicit conversion explicit then.
On 04/11/2015 15:07, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On 04/11/2015 12:05, Richard Henderson wrote: >>> On 11/04/2015 11:45 AM, Paolo Bonzini wrote: >>>>>>>>>> int32_t src = rs2 >> (word * 32); >>>>>>>>>> - int64_t scaled = src << scale; >>>>>>>>>> + int64_t scaled = (int64_t)src << scale; >>>>>>>>>> int64_t from_fixed = scaled >> 16; >>> ... >>>>> >>>>> I do think we'd be better served by casting to uint64_t on that line. >>>>> Note that fpackfix requires the same correction. And it wouldn't hurt >>>>> to cast to uint32_t in fpack16, lest we anger the self-same shifting >>>>> gods. >>>> >>>> Hmmm.. say src = -0x80000000, scale = 1; >>>> >>>> scaled = (uint64_t)-0x8000000 << 1 = 0xffffffff00000000 >>>> from_fixed = 0xffffffff00000000 >> 16 = 0x0000ffffffff0000 >>>> >>>> Now from_fixed is positive and you get 32767 instead of -32768. In >>>> other words, we would have to cast to uint64_t on the scaled assignment, >>>> and back to int64_t on the from_fixed assignment. I must be >>>> misunderstanding your suggestion. >>> >>> int64_t scaled = (uint64_t)src << scale; >>> >>> I.e. one explicit conversion and one implicit conversion. >> >> That does the job, but it also does look like a typo... > > Make the implicit conversion explicit then. Sorry, but I'll say it again: there's _no way_ that a sane compiler will _ever_ use this particular bit of undefined behavior. I'm generally against uglifying the code to placate ubsan, but especially so in this case: it is not common code and it would only affect people running fpackfix under ubsan. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 04/11/2015 15:07, Markus Armbruster wrote: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> On 04/11/2015 12:05, Richard Henderson wrote: >>>> On 11/04/2015 11:45 AM, Paolo Bonzini wrote: >>>>>>>>>>> int32_t src = rs2 >> (word * 32); >>>>>>>>>>> - int64_t scaled = src << scale; >>>>>>>>>>> + int64_t scaled = (int64_t)src << scale; >>>>>>>>>>> int64_t from_fixed = scaled >> 16; >>>> ... >>>>>> >>>>>> I do think we'd be better served by casting to uint64_t on that line. >>>>>> Note that fpackfix requires the same correction. And it wouldn't hurt >>>>>> to cast to uint32_t in fpack16, lest we anger the self-same shifting >>>>>> gods. >>>>> >>>>> Hmmm.. say src = -0x80000000, scale = 1; >>>>> >>>>> scaled = (uint64_t)-0x8000000 << 1 = 0xffffffff00000000 >>>>> from_fixed = 0xffffffff00000000 >> 16 = 0x0000ffffffff0000 >>>>> >>>>> Now from_fixed is positive and you get 32767 instead of -32768. In >>>>> other words, we would have to cast to uint64_t on the scaled assignment, >>>>> and back to int64_t on the from_fixed assignment. I must be >>>>> misunderstanding your suggestion. >>>> >>>> int64_t scaled = (uint64_t)src << scale; >>>> >>>> I.e. one explicit conversion and one implicit conversion. >>> >>> That does the job, but it also does look like a typo... >> >> Make the implicit conversion explicit then. > > Sorry, but I'll say it again: there's _no way_ that a sane compiler will > _ever_ use this particular bit of undefined behavior. > > I'm generally against uglifying the code to placate ubsan, but > especially so in this case: it is not common code and it would only > affect people running fpackfix under ubsan. Oh, I don't disagree at all with "let's not uglify code". I wish compilers hadn't become so nasty, though. I wish they had more respect for the imperfect real-world code they compile, and less benchmark worship.
On 04/11/15 11:05, Richard Henderson wrote: > On 11/04/2015 11:45 AM, Paolo Bonzini wrote: >>>>>>>> int32_t src = rs2 >> (word * 32); >>>>>>>> - int64_t scaled = src << scale; >>>>>>>> + int64_t scaled = (int64_t)src << scale; >>>>>>>> int64_t from_fixed = scaled >> 16; > ... >>> >>> I do think we'd be better served by casting to uint64_t on that line. >>> Note that fpackfix requires the same correction. And it wouldn't hurt >>> to cast to uint32_t in fpack16, lest we anger the self-same shifting >>> gods. >> >> Hmmm.. say src = -0x80000000, scale = 1; >> >> scaled = (uint64_t)-0x8000000 << 1 = 0xffffffff00000000 >> from_fixed = 0xffffffff00000000 >> 16 = 0x0000ffffffff0000 >> >> Now from_fixed is positive and you get 32767 instead of -32768. In >> other words, we would have to cast to uint64_t on the scaled assignment, >> and back to int64_t on the from_fixed assignment. I must be >> misunderstanding your suggestion. > > int64_t scaled = (uint64_t)src << scale; > > I.e. one explicit conversion and one implicit conversion. I suspect Richard knows more about this part of SPARC emulation than I do, so I'd be fine with a solution similar to the above if everyone agress. Let me know if you need me to send a SPARC pull request, although it will probably be quicker coming from Paolo/Richard at the moment. ATB, Mark.
On 04/11/2015 18:53, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On 04/11/2015 15:07, Markus Armbruster wrote: >>> Paolo Bonzini <pbonzini@redhat.com> writes: >>> >>>> On 04/11/2015 12:05, Richard Henderson wrote: >>>>> On 11/04/2015 11:45 AM, Paolo Bonzini wrote: >>>>>>>>>>>> int32_t src = rs2 >> (word * 32); >>>>>>>>>>>> - int64_t scaled = src << scale; >>>>>>>>>>>> + int64_t scaled = (int64_t)src << scale; >>>>>>>>>>>> int64_t from_fixed = scaled >> 16; >>>>> ... >>>>>>> >>>>>>> I do think we'd be better served by casting to uint64_t on that line. >>>>>>> Note that fpackfix requires the same correction. And it wouldn't hurt >>>>>>> to cast to uint32_t in fpack16, lest we anger the self-same shifting >>>>>>> gods. >>>>>> >>>>>> Hmmm.. say src = -0x80000000, scale = 1; >>>>>> >>>>>> scaled = (uint64_t)-0x8000000 << 1 = 0xffffffff00000000 >>>>>> from_fixed = 0xffffffff00000000 >> 16 = 0x0000ffffffff0000 >>>>>> >>>>>> Now from_fixed is positive and you get 32767 instead of -32768. In >>>>>> other words, we would have to cast to uint64_t on the scaled assignment, >>>>>> and back to int64_t on the from_fixed assignment. I must be >>>>>> misunderstanding your suggestion. >>>>> >>>>> int64_t scaled = (uint64_t)src << scale; >>>>> >>>>> I.e. one explicit conversion and one implicit conversion. >>>> >>>> That does the job, but it also does look like a typo... >>> >>> Make the implicit conversion explicit then. >> >> Sorry, but I'll say it again: there's _no way_ that a sane compiler will >> _ever_ use this particular bit of undefined behavior. >> >> I'm generally against uglifying the code to placate ubsan, but >> especially so in this case: it is not common code and it would only >> affect people running fpackfix under ubsan. > > Oh, I don't disagree at all with "let's not uglify code". > > I wish compilers hadn't become so nasty, though. I wish they had more > respect for the imperfect real-world code they compile, and less > benchmark worship. It's not benchmark worship. For example, being able to optimize "x * 6 / 2" to "x * 3" is useful, but it's only possible if you can treat integer overflow as undefined. In fact GCC compiles it to leal (%rdi,%rdi,2), %eax add r0, r0, r0, lsl #1 (x86 and ARM respectively) for int, and to leal (%rdi,%rdi,2), %eax mov r3, r0, asl #3 andl $2147483647, %eax sub r0, r3, r0, asl #1 mov r0, r0, lsr #1 for unsigned int. This is less efficient; stuff like this happens in *real programs* and even in hot loops. For an even more extreme case, "x * 10000000 / 1000000" with int and unsigned: leal (%rdi,%rdi,4), %eax mov r3, r0, asl #3 addl %eax, %eax add r0, r3, r0, lsl #1 vs. imull $10000000, %edi, %edx movw r3, #38528 movl $1125899907, %ecx movw r2, #56963 movl %edx, %eax movt r3, 152 mull %ecx movt r2, 17179 movl %edx, %eax mul r0, r3, r0 shrl $18, %eax umull r0, r1, r0, r2 mov r0, r1, lsr #18 (For completeness I'll note that this optimization may also _hide_ bugs on ints, which can be both a good or a bad thing; compiler warnings and static analysis can help fixing the code). Similarly for optimizing for (i = 0; i <= n; i++) p[i] = 0; to any of for (q = p, r = p + n; q <= r; q++) *q = 0; or for (q = p, i = n + 1; i-- > 0; q++) *q = 0; Both of which are cases of strength reduction that are expected by any optimizing compiler (without even going into vectorization). Yet they are not possible without treating overflow as undefined. The last may seem strange, but it's easy for a compiler to look at the original loop and derive that it has n + 1 iterations. This can be used with hardware loop counter registers (e.g. CTR on PowerPC) or decrement-and-loop instructions (e.g. bdnz on PowerPC, loop on x86). DSP code, for example, is full of code like this where n is known at compile time, and one would have to write assembly code if the compiler didn't about these instruction. As for the so-much-loathed type-based alias analysis, it lets you optimize this: void f(float **a, float **b, int m, int n) { int i, j; for (i = 0; i < m; i++) for (j = 0; j < n; j++) b[i][j] = a[i][j]; } to this: void f(float **a, float **b, int m, int n) { int i, j; for (i = 0; i < m; i++) { float *ai = a[i], *bi = b[i]; for (j = 0; j < n; j++) bi[j] = ai[j]; } } Compiler writers don't rely on undefined behavior out of spite. There _is_ careful analysis of what kind of code could be broken by it, and typically there are also warning mechanisms (-Wstrict-aliasing, -Wstrict-overflow, -Wunsafe-loop-optimizations) to help the programmer. It's not a coincidence that left shifting of signed negative numbers a) is not relied on by neither GCC nor clang; b) is the source of the wide majority of ubsan failures for QEMU. Paolo
On 05/11/2015 00:36, Mark Cave-Ayland wrote: > On 04/11/15 11:05, Richard Henderson wrote: > >> On 11/04/2015 11:45 AM, Paolo Bonzini wrote: >>>>>>>>> int32_t src = rs2 >> (word * 32); >>>>>>>>> - int64_t scaled = src << scale; >>>>>>>>> + int64_t scaled = (int64_t)src << scale; >>>>>>>>> int64_t from_fixed = scaled >> 16; >> ... >>>> >>>> I do think we'd be better served by casting to uint64_t on that line. >>>> Note that fpackfix requires the same correction. And it wouldn't hurt >>>> to cast to uint32_t in fpack16, lest we anger the self-same shifting >>>> gods. >>> >>> Hmmm.. say src = -0x80000000, scale = 1; >>> >>> scaled = (uint64_t)-0x8000000 << 1 = 0xffffffff00000000 >>> from_fixed = 0xffffffff00000000 >> 16 = 0x0000ffffffff0000 >>> >>> Now from_fixed is positive and you get 32767 instead of -32768. In >>> other words, we would have to cast to uint64_t on the scaled assignment, >>> and back to int64_t on the from_fixed assignment. I must be >>> misunderstanding your suggestion. >> >> int64_t scaled = (uint64_t)src << scale; >> >> I.e. one explicit conversion and one implicit conversion. > > I suspect Richard knows more about this part of SPARC emulation than I > do, so I'd be fine with a solution similar to the above if everyone > agress. Let me know if you need me to send a SPARC pull request, > although it will probably be quicker coming from Paolo/Richard at the > moment. All solutions work. You have to tell us which you prefer among /* Has undefined behavior (though no compiler uses it) */ int64_t scaled = (int64_t)src << scale; /* Seems like a typo */ int64_t scaled = (uint64_t)src << scale; /* Ugly code */ int64_t scaled = (uint64_t)(int64_t)src << scale; Paolo
On 11/05/2015 10:12 AM, Paolo Bonzini wrote: > /* Ugly code */ > int64_t scaled = (uint64_t)(int64_t)src << scale; You mean int64_t scaled = (int64_t)((uint64_t)src << scale); r~
On 05/11/2015 10:20, Richard Henderson wrote: > >> /* Ugly code */ >> int64_t scaled = (uint64_t)(int64_t)src << scale; > > You mean > > int64_t scaled = (int64_t)((uint64_t)src << scale); No, that also looks like a typo. I mean: - unnecessary cast to int64_t to get the sign extension while avoiding the impression of a typo - cast to uint64_t to avoid overflow - the shift is done in the uint64_t type - finally there is an implicit cast to int64_t Paolo
On 11/05/2015 10:25 AM, Paolo Bonzini wrote: > > > On 05/11/2015 10:20, Richard Henderson wrote: >> >>> /* Ugly code */ >>> int64_t scaled = (uint64_t)(int64_t)src << scale; >> >> You mean >> >> int64_t scaled = (int64_t)((uint64_t)src << scale); > > No, that also looks like a typo. > > I mean: > > - unnecessary cast to int64_t to get the sign extension while avoiding > the impression of a typo Huh. This part doesn't seem a typo to me at all. > > - cast to uint64_t to avoid overflow > > - the shift is done in the uint64_t type > > - finally there is an implicit cast to int64_t r~
On 05/11/2015 10:28, Richard Henderson wrote: > On 11/05/2015 10:25 AM, Paolo Bonzini wrote: >> >> >> On 05/11/2015 10:20, Richard Henderson wrote: >>> >>>> /* Ugly code */ >>>> int64_t scaled = (uint64_t)(int64_t)src << scale; >>> >>> You mean >>> >>> int64_t scaled = (int64_t)((uint64_t)src << scale); >> >> No, that also looks like a typo. >> >> I mean: >> >> - unnecessary cast to int64_t to get the sign extension while avoiding >> the impression of a typo > > Huh. This part doesn't seem a typo to me at all. A cast _is_ obviously necessary, because src is int32_t and the result is int64_t: int32_t src = rs2 >> (word * 32); int64_t scaled = (uint64_t)src << scale; having uint64_t on the RHS and int64_t on the LHS definitely would be a WTF cause for me. Paolo
On 05/11/15 09:25, Paolo Bonzini wrote: > On 05/11/2015 10:20, Richard Henderson wrote: >> >>> /* Ugly code */ >>> int64_t scaled = (uint64_t)(int64_t)src << scale; >> >> You mean >> >> int64_t scaled = (int64_t)((uint64_t)src << scale); > > No, that also looks like a typo. > > I mean: > > - unnecessary cast to int64_t to get the sign extension while avoiding > the impression of a typo > > - cast to uint64_t to avoid overflow > > - the shift is done in the uint64_t type > > - finally there is an implicit cast to int64_t I would say that Richard's version above is the most readable to me, however from what you're saying this would cause the compiler to produce much less efficient code? If this is the case then I could live with your second choice ("Seems like a typo") with an appropriate comment if this maintains the efficiency of generated code whilst also having well-defined behaviour between compilers. Out of interest has anyone tried these alternatives on clang? ATB, Mark.
On 06/11/2015 16:33, Mark Cave-Ayland wrote: >>> >> >>>> >>> /* Ugly code */ >>>> >>> int64_t scaled = (uint64_t)(int64_t)src << scale; >>> >> >>> >> You mean >>> >> >>> >> int64_t scaled = (int64_t)((uint64_t)src << scale); >> > >> > No, that also looks like a typo. >> > >> > I mean: >> > >> > - unnecessary cast to int64_t to get the sign extension while avoiding >> > the impression of a typo >> > >> > - cast to uint64_t to avoid overflow >> > >> > - the shift is done in the uint64_t type >> > >> > - finally there is an implicit cast to int64_t > I would say that Richard's version above is the most readable to me, > however from what you're saying this would cause the compiler to produce > much less efficient code? No, they should all be the same. Let's go with the "seems like a typo" version :) with a comment to say that no, it's not a typo. Paolo > If this is the case then I could live with your second choice ("Seems > like a typo") with an appropriate comment if this maintains the > efficiency of generated code whilst also having well-defined behaviour > between compilers. Out of interest has anyone tried these alternatives > on clang?
diff --git a/target-sparc/vis_helper.c b/target-sparc/vis_helper.c index 383cc8b..45fc7db 100644 --- a/target-sparc/vis_helper.c +++ b/target-sparc/vis_helper.c @@ -447,7 +447,7 @@ uint32_t helper_fpackfix(uint64_t gsr, uint64_t rs2) for (word = 0; word < 2; word++) { uint32_t val; int32_t src = rs2 >> (word * 32); - int64_t scaled = src << scale; + int64_t scaled = (int64_t)src << scale; int64_t from_fixed = scaled >> 16; val = (from_fixed < -32768 ? -32768 :
This is reported by Coverity. The algorithm description at ftp://ftp.icm.edu.pl/packages/ggi/doc/hw/sparc/Sparc.pdf suggests that the 32-bit parts of rs2, after the left shift, is treated as a 64-bit integer. Bits 32 and above are used to do the saturating truncation. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- target-sparc/vis_helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)