Re: [Patch] sext_hwi: Avoid left shift of negative value undefined behaviour

[Jeff Law <law at redhat dot com>]

On 08/12/2015 07:33 AM, Mikael Morin wrote:
> Le 12/08/2015 13:09, Richard Biener a Ãcrit :
> > On Wed, Aug 12, 2015 at 1:07 PM, Markus Trippelsdorf
> > <markus@trippelsdorf.de> wrote:
> > > On 2015.08.12 at 13:01 +0200, Richard Biener wrote:
> > > > What kind of error does ubsan run into?  That is, for which 'prec'?
> > >
> > > See: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67042
> >
> > Ugh.  Stupid C.
> >
> > My fear is that with so many stmts sext_hwi isn't any longer
> > considered for
> > inlining, even if prec is constant.  What's the generated code for its
> > out-of line
> > copy with/without the patch?
>
> The difference on x86_64 is:
>
>         .cfi_startproc
>         cmpl    $64, %esi
>         je    .L3
> -    movl    $64, %ecx
> -    subl    %esi, %ecx
> -    salq    %cl, %rdi
> +    leal    -1(%rsi), %ecx
> +    movl    $1, %eax
> +    movq    %rax, %rdx
> +    salq    %cl, %rdx
> +    movl    %esi, %ecx
> +    salq    %cl, %rax
> +    subq    $1, %rax
> +    andq    %rax, %rdi
> +    xorq    %rdx, %rdi
>         movq    %rdi, %rax
> -    sarq    %cl, %rax
> +    subq    %rdx, %rax
>         ret
>         .p2align 4,,10
>         .p2align 3
>
> with -O2, tests attached.
> Yes it's worse.  I thought it was better to avoid undefined behaviour at
> all prices to avoid "bad surprises".
Well, this isn't the right test.  You're testing when PREC is an unknown 
and I fully expect in that case the code you're going to get will be 
worse.  There's too many insns for combine to save us in that case.

What's interesting here is what happens when prec is a known value 
(since we're hoping that's the common case via inlining).

When prec has a known value of 8, 16 or 32 we get a nice movs[bwl]q on 
x86_64, which is exactly what we want.

When prec is another known value, say 13 for the sake of argument, we 
get a pair of shifts, which is again exactly what we want.

So when prec is a constant, we're going to get good code.  So the only 
question left is whether or not prec is usually a constant or not in the 
contexts in which this routine is used.


Jeff