Re: linux/bitops.h

[H. Peter Anvin]

On May 6, 2016 1:07:13 PM PDT, Sasha Levin  wrote:
>On 05/04/2016 08:30 PM, H. Peter Anvin wrote:
>> On 05/04/16 15:06, John Denker wrote:
>>> On 05/04/2016 02:56 PM, H. Peter Anvin wrote:
> Beware that shifting by an amount >= the number of bits in the
> word remains Undefined Behavior.
>>>
 This construct has been supported as a rotate since at least gcc2.
>>>
>>> How then should we understand the story told in commit d7e35dfa?
>>> Is the story wrong?
>>>
>>> At the very least, something inconsistent is going on.  There
>>> are 8 functions.  Why did d7e35dfa change one of them but
>>> not the other 7?
>> 
>> Yes. d7e35dfa is baloney IMNSHO.  All it does is produce worse code,
>and the description even says so.
>
>No, the description says that it produces worse code for *really
>really* ancient
>GCC versions.
>
>> As I said, gcc has treated the former code as idiomatic since gcc 2,
>so that support is beyond ancient.
>
>Because something works in a specific way on one compiler isn't a
>reason to
>ignore this noncompliance with the standard.
>
>
>Thanks,
>Sasha

When the compiler in question is our flagship target and our reference 
compiler, then yes, it matters.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

Re: linux/bitops.h

[H. Peter Anvin]

On May 6, 2016 1:07:13 PM PDT, Sasha Levin  wrote:
>On 05/04/2016 08:30 PM, H. Peter Anvin wrote:
>> On 05/04/16 15:06, John Denker wrote:
>>> On 05/04/2016 02:56 PM, H. Peter Anvin wrote:
> Beware that shifting by an amount >= the number of bits in the
> word remains Undefined Behavior.
>>>
 This construct has been supported as a rotate since at least gcc2.
>>>
>>> How then should we understand the story told in commit d7e35dfa?
>>> Is the story wrong?
>>>
>>> At the very least, something inconsistent is going on.  There
>>> are 8 functions.  Why did d7e35dfa change one of them but
>>> not the other 7?
>> 
>> Yes. d7e35dfa is baloney IMNSHO.  All it does is produce worse code,
>and the description even says so.
>
>No, the description says that it produces worse code for *really
>really* ancient
>GCC versions.
>
>> As I said, gcc has treated the former code as idiomatic since gcc 2,
>so that support is beyond ancient.
>
>Because something works in a specific way on one compiler isn't a
>reason to
>ignore this noncompliance with the standard.
>
>
>Thanks,
>Sasha

When the compiler in question is our flagship target and our reference 
compiler, then yes, it matters.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

Re: linux/bitops.h

[H. Peter Anvin]

On May 6, 2016 1:07:13 PM PDT, Sasha Levin  wrote:
>On 05/04/2016 08:30 PM, H. Peter Anvin wrote:
>> On 05/04/16 15:06, John Denker wrote:
>>> On 05/04/2016 02:56 PM, H. Peter Anvin wrote:
> Beware that shifting by an amount >= the number of bits in the
> word remains Undefined Behavior.
>>>
 This construct has been supported as a rotate since at least gcc2.
>>>
>>> How then should we understand the story told in commit d7e35dfa?
>>> Is the story wrong?
>>>
>>> At the very least, something inconsistent is going on.  There
>>> are 8 functions.  Why did d7e35dfa change one of them but
>>> not the other 7?
>> 
>> Yes. d7e35dfa is baloney IMNSHO.  All it does is produce worse code,
>and the description even says so.
>
>No, the description says that it produces worse code for *really
>really* ancient
>GCC versions.
>
>> As I said, gcc has treated the former code as idiomatic since gcc 2,
>so that support is beyond ancient.
>
>Because something works in a specific way on one compiler isn't a
>reason to
>ignore this noncompliance with the standard.
>
>
>Thanks,
>Sasha

4.6.2 is not "really, really ancient."
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

Re: linux/bitops.h

[H. Peter Anvin]

On May 6, 2016 1:07:13 PM PDT, Sasha Levin  wrote:
>On 05/04/2016 08:30 PM, H. Peter Anvin wrote:
>> On 05/04/16 15:06, John Denker wrote:
>>> On 05/04/2016 02:56 PM, H. Peter Anvin wrote:
> Beware that shifting by an amount >= the number of bits in the
> word remains Undefined Behavior.
>>>
 This construct has been supported as a rotate since at least gcc2.
>>>
>>> How then should we understand the story told in commit d7e35dfa?
>>> Is the story wrong?
>>>
>>> At the very least, something inconsistent is going on.  There
>>> are 8 functions.  Why did d7e35dfa change one of them but
>>> not the other 7?
>> 
>> Yes. d7e35dfa is baloney IMNSHO.  All it does is produce worse code,
>and the description even says so.
>
>No, the description says that it produces worse code for *really
>really* ancient
>GCC versions.
>
>> As I said, gcc has treated the former code as idiomatic since gcc 2,
>so that support is beyond ancient.
>
>Because something works in a specific way on one compiler isn't a
>reason to
>ignore this noncompliance with the standard.
>
>
>Thanks,
>Sasha

4.6.2 is not "really, really ancient."
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

Re: linux/bitops.h

[Sasha Levin]

On 05/04/2016 08:48 PM, Linus Torvalds wrote:
> That said, the fact that the other cases weren't changed
> (rol64/ror64/ror32) does make that argument less interesting. Unless
> there was some particular code that actively ended up using
> "rol32(..0)" but not the other cases.

Right, the others seemed wrong as well but I couldn't find any code
that triggers that, and preferred to fix just the one I was hitting.

I can go fix the rest if that's something we want to do?


Thanks,
Sasha

Re: linux/bitops.h

[Sasha Levin]

On 05/04/2016 08:48 PM, Linus Torvalds wrote:
> That said, the fact that the other cases weren't changed
> (rol64/ror64/ror32) does make that argument less interesting. Unless
> there was some particular code that actively ended up using
> "rol32(..0)" but not the other cases.

Right, the others seemed wrong as well but I couldn't find any code
that triggers that, and preferred to fix just the one I was hitting.

I can go fix the rest if that's something we want to do?


Thanks,
Sasha

Re: linux/bitops.h

[Sasha Levin]

On 05/04/2016 08:30 PM, H. Peter Anvin wrote:
> On 05/04/16 15:06, John Denker wrote:
>> On 05/04/2016 02:56 PM, H. Peter Anvin wrote:
 Beware that shifting by an amount >= the number of bits in the
 word remains Undefined Behavior.
>>
>>> This construct has been supported as a rotate since at least gcc2.
>>
>> How then should we understand the story told in commit d7e35dfa?
>> Is the story wrong?
>>
>> At the very least, something inconsistent is going on.  There
>> are 8 functions.  Why did d7e35dfa change one of them but
>> not the other 7?
> 
> Yes. d7e35dfa is baloney IMNSHO.  All it does is produce worse code, and the 
> description even says so.

No, the description says that it produces worse code for *really really* ancient
GCC versions.

> As I said, gcc has treated the former code as idiomatic since gcc 2, so that 
> support is beyond ancient.

Because something works in a specific way on one compiler isn't a reason to
ignore this noncompliance with the standard.


Thanks,
Sasha

Re: linux/bitops.h

[Sasha Levin]

On 05/04/2016 08:30 PM, H. Peter Anvin wrote:
> On 05/04/16 15:06, John Denker wrote:
>> On 05/04/2016 02:56 PM, H. Peter Anvin wrote:
 Beware that shifting by an amount >= the number of bits in the
 word remains Undefined Behavior.
>>
>>> This construct has been supported as a rotate since at least gcc2.
>>
>> How then should we understand the story told in commit d7e35dfa?
>> Is the story wrong?
>>
>> At the very least, something inconsistent is going on.  There
>> are 8 functions.  Why did d7e35dfa change one of them but
>> not the other 7?
> 
> Yes. d7e35dfa is baloney IMNSHO.  All it does is produce worse code, and the 
> description even says so.

No, the description says that it produces worse code for *really really* ancient
GCC versions.

> As I said, gcc has treated the former code as idiomatic since gcc 2, so that 
> support is beyond ancient.

Because something works in a specific way on one compiler isn't a reason to
ignore this noncompliance with the standard.


Thanks,
Sasha

Re: linux/bitops.h

[H. Peter Anvin]

On May 4, 2016 6:20:32 PM PDT, Jeffrey Walton  wrote:
>On Wed, May 4, 2016 at 7:06 PM, Andi Kleen  wrote:
>> On Wed, May 04, 2016 at 03:06:04PM -0700, John Denker wrote:
>>> On 05/04/2016 02:56 PM, H. Peter Anvin wrote:
>>> >> Beware that shifting by an amount >= the number of bits in the
>>> >> word remains Undefined Behavior.
>>>
>>> > This construct has been supported as a rotate since at least gcc2.
>>>
>>> How then should we understand the story told in commit d7e35dfa?
>>> Is the story wrong?
>>
>> I don't think Linux runs on a system where it would make a difference
>> (like a VAX), and also gcc always converts it before it could.
>> Even UBSan should not complain because it runs after the conversion
>> to ROTATE.
>>
>From what I understand, its a limitation in the barrel shifter and the
>way the shift bits are handled.
>
>Linux runs on a great number of devices, so its conceivable (likely?)
>a low-cost board would have hardware limitations that not found in
>modern desktops and servers or VAX.
>
>Jeff

This is a compiler feature, though.  And if icc or clang don't support the 
idiom they would never have compiled a working kernel.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

Re: linux/bitops.h

[H. Peter Anvin]

On May 4, 2016 6:20:32 PM PDT, Jeffrey Walton  wrote:
>On Wed, May 4, 2016 at 7:06 PM, Andi Kleen  wrote:
>> On Wed, May 04, 2016 at 03:06:04PM -0700, John Denker wrote:
>>> On 05/04/2016 02:56 PM, H. Peter Anvin wrote:
>>> >> Beware that shifting by an amount >= the number of bits in the
>>> >> word remains Undefined Behavior.
>>>
>>> > This construct has been supported as a rotate since at least gcc2.
>>>
>>> How then should we understand the story told in commit d7e35dfa?
>>> Is the story wrong?
>>
>> I don't think Linux runs on a system where it would make a difference
>> (like a VAX), and also gcc always converts it before it could.
>> Even UBSan should not complain because it runs after the conversion
>> to ROTATE.
>>
>From what I understand, its a limitation in the barrel shifter and the
>way the shift bits are handled.
>
>Linux runs on a great number of devices, so its conceivable (likely?)
>a low-cost board would have hardware limitations that not found in
>modern desktops and servers or VAX.
>
>Jeff

This is a compiler feature, though.  And if icc or clang don't support the 
idiom they would never have compiled a working kernel.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

Re: linux/bitops.h

[Jeffrey Walton]

On Wed, May 4, 2016 at 7:06 PM, Andi Kleen  wrote:
> On Wed, May 04, 2016 at 03:06:04PM -0700, John Denker wrote:
>> On 05/04/2016 02:56 PM, H. Peter Anvin wrote:
>> >> Beware that shifting by an amount >= the number of bits in the
>> >> word remains Undefined Behavior.
>>
>> > This construct has been supported as a rotate since at least gcc2.
>>
>> How then should we understand the story told in commit d7e35dfa?
>> Is the story wrong?
>
> I don't think Linux runs on a system where it would make a difference
> (like a VAX), and also gcc always converts it before it could.
> Even UBSan should not complain because it runs after the conversion
> to ROTATE.
>
>From what I understand, its a limitation in the barrel shifter and the
way the shift bits are handled.

Linux runs on a great number of devices, so its conceivable (likely?)
a low-cost board would have hardware limitations that not found in
modern desktops and servers or VAX.

Jeff

Re: linux/bitops.h

[Jeffrey Walton]

On Wed, May 4, 2016 at 7:06 PM, Andi Kleen  wrote:
> On Wed, May 04, 2016 at 03:06:04PM -0700, John Denker wrote:
>> On 05/04/2016 02:56 PM, H. Peter Anvin wrote:
>> >> Beware that shifting by an amount >= the number of bits in the
>> >> word remains Undefined Behavior.
>>
>> > This construct has been supported as a rotate since at least gcc2.
>>
>> How then should we understand the story told in commit d7e35dfa?
>> Is the story wrong?
>
> I don't think Linux runs on a system where it would make a difference
> (like a VAX), and also gcc always converts it before it could.
> Even UBSan should not complain because it runs after the conversion
> to ROTATE.
>
>From what I understand, its a limitation in the barrel shifter and the
way the shift bits are handled.

Linux runs on a great number of devices, so its conceivable (likely?)
a low-cost board would have hardware limitations that not found in
modern desktops and servers or VAX.

Jeff

Re: linux/bitops.h

[Linus Torvalds]

On Wed, May 4, 2016 at 5:30 PM, H. Peter Anvin  wrote:
>
> Yes. d7e35dfa is baloney IMNSHO.  All it does is produce worse code, and the
> description even says so.
>
> As I said, gcc has treated the former code as idiomatic since gcc 2, so that
> support is beyond ancient.

Well, we're *trying* to get clang supported, so the argument that gcc
has always supported it and compiles correct code for it is not
necessarily the whole story yet.

The problem with "32 - shift" is that it really could generate garbage
in situations where shift ends up being a constant zero..

That said, the fact that the other cases weren't changed
(rol64/ror64/ror32) does make that argument less interesting. Unless
there was some particular code that actively ended up using
"rol32(..0)" but not the other cases.

(And yes, rol32(x,0) is obviously nonsense, but it could easily happen
with inline functions or macros that end up generating that).

Note that there may be 8 "rol/ror" functions, but the 16-bit and 8-bit
ones don't have the undefined semantics. So there are only four that
had _that_ problem, although I agree that changing just one out of
four is wrong.

Linus

Re: linux/bitops.h

[Linus Torvalds]

On Wed, May 4, 2016 at 5:30 PM, H. Peter Anvin  wrote:
>
> Yes. d7e35dfa is baloney IMNSHO.  All it does is produce worse code, and the
> description even says so.
>
> As I said, gcc has treated the former code as idiomatic since gcc 2, so that
> support is beyond ancient.

Well, we're *trying* to get clang supported, so the argument that gcc
has always supported it and compiles correct code for it is not
necessarily the whole story yet.

The problem with "32 - shift" is that it really could generate garbage
in situations where shift ends up being a constant zero..

That said, the fact that the other cases weren't changed
(rol64/ror64/ror32) does make that argument less interesting. Unless
there was some particular code that actively ended up using
"rol32(..0)" but not the other cases.

(And yes, rol32(x,0) is obviously nonsense, but it could easily happen
with inline functions or macros that end up generating that).

Note that there may be 8 "rol/ror" functions, but the 16-bit and 8-bit
ones don't have the undefined semantics. So there are only four that
had _that_ problem, although I agree that changing just one out of
four is wrong.

Linus

Re: linux/bitops.h

[H. Peter Anvin]

On 05/04/16 15:06, John Denker wrote:

On 05/04/2016 02:56 PM, H. Peter Anvin wrote:

Beware that shifting by an amount >= the number of bits in the
word remains Undefined Behavior.



This construct has been supported as a rotate since at least gcc2.


How then should we understand the story told in commit d7e35dfa?
Is the story wrong?

At the very least, something inconsistent is going on.  There
are 8 functions.  Why did d7e35dfa change one of them but
not the other 7?


Yes. d7e35dfa is baloney IMNSHO.  All it does is produce worse code, and 
the description even says so.


As I said, gcc has treated the former code as idiomatic since gcc 2, so 
that support is beyond ancient.


-hpa

Re: linux/bitops.h

[H. Peter Anvin]

On 05/04/16 15:06, John Denker wrote:

On 05/04/2016 02:56 PM, H. Peter Anvin wrote:

Beware that shifting by an amount >= the number of bits in the
word remains Undefined Behavior.



This construct has been supported as a rotate since at least gcc2.


How then should we understand the story told in commit d7e35dfa?
Is the story wrong?

At the very least, something inconsistent is going on.  There
are 8 functions.  Why did d7e35dfa change one of them but
not the other 7?


Yes. d7e35dfa is baloney IMNSHO.  All it does is produce worse code, and 
the description even says so.


As I said, gcc has treated the former code as idiomatic since gcc 2, so 
that support is beyond ancient.


-hpa

Re: linux/bitops.h

[John Denker]

On 05/04/2016 04:06 PM, Andi Kleen wrote:

> gcc always converts it before it could
[make a difference].

At the moment, current versions of gcc treat the idiomatic
ror/rol code as something they support ... but older versions
do not, and future version may not.

The gcc guys have made it very clear that they reserve the
right to do absolutely anything they want in a UB situation.
 -- What is true as of today might not be "always" true.
 -- What is true at one level of optimization might not be
  true at another.
 -- The consequences can be highly nonlocal and counterintuitive.
  For example, in the case of:
 rslt = word << (32 - N);
 ...
 ...
 if (!N) { ... }
  the compiler could assume that N is necessarily nonzero,
  and many lines later it could optimize out the whole
  if-block.  So, even if the "<<" operator gives the right
  result, there could be ghastly failures elsewhere.  It
  might work for some people but not others.

> So it's unlikely to be a pressing issue.

Sometimes issues that are not urgently "pressing" ought
to be dealt with in a systematic way.

There are serious people who think that avoiding UB is
a necessity, if you want the code to be reliable and
maintainable.

I renew the question:  Why did commit d7e35dfa upgrade
one of the 8 functions but not the other 7?
  -- I could understand 0 of 8, or 8 of 8.
  -- In contrast, I'm having a hard time understanding
   why 7 of the 8 use the idiomatic expression while the
   8th does not.

Re: linux/bitops.h

[John Denker]

On 05/04/2016 04:06 PM, Andi Kleen wrote:

> gcc always converts it before it could
[make a difference].

At the moment, current versions of gcc treat the idiomatic
ror/rol code as something they support ... but older versions
do not, and future version may not.

The gcc guys have made it very clear that they reserve the
right to do absolutely anything they want in a UB situation.
 -- What is true as of today might not be "always" true.
 -- What is true at one level of optimization might not be
  true at another.
 -- The consequences can be highly nonlocal and counterintuitive.
  For example, in the case of:
 rslt = word << (32 - N);
 ...
 ...
 if (!N) { ... }
  the compiler could assume that N is necessarily nonzero,
  and many lines later it could optimize out the whole
  if-block.  So, even if the "<<" operator gives the right
  result, there could be ghastly failures elsewhere.  It
  might work for some people but not others.

> So it's unlikely to be a pressing issue.

Sometimes issues that are not urgently "pressing" ought
to be dealt with in a systematic way.

There are serious people who think that avoiding UB is
a necessity, if you want the code to be reliable and
maintainable.

I renew the question:  Why did commit d7e35dfa upgrade
one of the 8 functions but not the other 7?
  -- I could understand 0 of 8, or 8 of 8.
  -- In contrast, I'm having a hard time understanding
   why 7 of the 8 use the idiomatic expression while the
   8th does not.

Re: linux/bitops.h

[Andi Kleen]

On Wed, May 04, 2016 at 03:06:04PM -0700, John Denker wrote:
> On 05/04/2016 02:56 PM, H. Peter Anvin wrote:
> >> Beware that shifting by an amount >= the number of bits in the
> >> word remains Undefined Behavior.
> 
> > This construct has been supported as a rotate since at least gcc2.
> 
> How then should we understand the story told in commit d7e35dfa?
> Is the story wrong?

I don't think Linux runs on a system where it would make a difference
(like a VAX), and also gcc always converts it before it could.
Even UBSan should not complain because it runs after the conversion
to ROTATE.

So it's unlikely to be a pressing issue.

-Andi

-- 
a...@linux.intel.com -- Speaking for myself only.

Re: linux/bitops.h

[Andi Kleen]

On Wed, May 04, 2016 at 03:06:04PM -0700, John Denker wrote:
> On 05/04/2016 02:56 PM, H. Peter Anvin wrote:
> >> Beware that shifting by an amount >= the number of bits in the
> >> word remains Undefined Behavior.
> 
> > This construct has been supported as a rotate since at least gcc2.
> 
> How then should we understand the story told in commit d7e35dfa?
> Is the story wrong?

I don't think Linux runs on a system where it would make a difference
(like a VAX), and also gcc always converts it before it could.
Even UBSan should not complain because it runs after the conversion
to ROTATE.

So it's unlikely to be a pressing issue.

-Andi

-- 
a...@linux.intel.com -- Speaking for myself only.

Re: linux/bitops.h

[John Denker]

On 05/04/2016 02:56 PM, H. Peter Anvin wrote:
>> Beware that shifting by an amount >= the number of bits in the
>> word remains Undefined Behavior.

> This construct has been supported as a rotate since at least gcc2.

How then should we understand the story told in commit d7e35dfa?
Is the story wrong?

At the very least, something inconsistent is going on.  There
are 8 functions.  Why did d7e35dfa change one of them but
not the other 7?

Re: linux/bitops.h

[John Denker]

On 05/04/2016 02:56 PM, H. Peter Anvin wrote:
>> Beware that shifting by an amount >= the number of bits in the
>> word remains Undefined Behavior.

> This construct has been supported as a rotate since at least gcc2.

How then should we understand the story told in commit d7e35dfa?
Is the story wrong?

At the very least, something inconsistent is going on.  There
are 8 functions.  Why did d7e35dfa change one of them but
not the other 7?