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

[Richard Biener <richard dot guenther at gmail dot com>]

On August 12, 2015 7:02:04 PM GMT+02:00, Jeff Law <law@redhat.com> wrote:
>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.

Prec is almost never a constant and is heavily used from wide-int.

We are not exploiting this undefined ness in C so I object to making this so much slower.

Can we instead do what we do for abs_hwi and add a checking assert so we can move the tests to the callers that misbehave instead?

Thanks,
Richard.

>
>Jeff