On 01/24/18 16:09, Peter Dimov via Boost wrote:

Andrey Semashev wrote:

...

My only ask is that the new names be concise, if possible.

The concisest I can come up with is sub_and_check_if_zero; anything else either doesn't read correctly, or is still ambiguous.

Maybe sub_and_test_zero then? So, do I understand correctly that, in your opinion, the current naming is confusing and should be changed?

On 01/25/18 14:32, Rob Stewart via Boost wrote:

On January 24, 2018 6:32:01 PM EST, Gavin Lambert via Boost wrote:

...

On 25/01/2018 00:35, Andrey Semashev wrote:

...

I would like to ask for the community opinion on the naming of the (op)_and_test functions that appeared in Boost.Atomic 1.66.

For clarity, I was the one who filed the issue mentioned in the OP.

My argument is the following:

* "if (x)" is true when x is an integer type that is nonzero. (And this convention is often extended to non-integer types as well, for suitable definitions of "zero".) * "atomic_flag::test_and_set" is true when the flag was previously set. * "atomic<T>::bit_test_and_set" is true when the bit was previously 1/set. * "atomic<T>::fetch_add" returns the value prior to the add, which is true if nonzero due to the first rule.

It thus seems peculiar to have "atomic<>::add_and_test" return true when the result is zero.

Your consistency argument seems compelling, unless there are unidentified counterexamples.

I suppose, I can add one other example supporting that pattern: "std::bitset<N>::test". Also, the standard includes wording such as "returns a value testable as true" when it describes predicates that can be used with algorithms, which means that the value can be contextually converted to bool (i.e. true if not "zero"). It seems like the term "test" indeed implies "check if something is not zero".

On January 25, 2018 9:51:16 AM EST, Peter Dimov via Boost wrote:

Andrey Semashev wrote:

...

It seems like the term "test" indeed implies "check if something is not zero".

It doesn't. Testing a _bit_ does test whether the bit is 1. Testing a condition does not imply that the condition "is not zero" is preferred over everything else. The x86 `test` instruction for example sets all condition codes, and it's up to the following conditional branch (jz, jnz, js, jns) to determine which one is being tested. (Ironically in the `jz` case it still tests whether a bit is 1 - the `zf` bit.)

But in any event, you can't settle an empirical question by reasoning. It's either confusing (to the actual audience) or it isn't, and if it is, elaborate rationalizations about why it shouldn't be do not change the outcome. You could just avoid that whole debate by making the name less ambiguous.

Consistency within this domain should be sufficient. If these functions, once made consistent, are confusing, all the others using "test" will be confusing, too. All must be renamed to solve that problem. -- Rob (Sent from my portable computation device.)

On Thu, Jan 25, 2018 at 6:03 PM, Peter Dimov via Boost wrote:

Consistency and intuition are in conflict here. These function are presently specified the way they are specified for a reason - this is what makes more sense in the context in which they are typically used. You can use consistency to justify a choice, but you can't magically make people not be confused by the choice.

This is similar to the bool conversion of std::error_code, which is also perfectly consistent and still manages to return the opposite of what a certain fraction of the audience intuitively expects.

I haven't been following this specific discussion closely, but in general, I wish designers put a higher value on consistency than on trying to guess what the audience's expectations are. Consistency leads to tools whose behavior can be reasoned about and competence with can be achieved. Guessing what the people want leads to incomprehensible black boxes that the user pokes at, hoping it will guess what they want it to do. -- Frank

On January 25, 2018 6:59:52 PM EST, Peter Dimov via Boost wrote:

Frank Mori Hess wrote:

...

I haven't been following this specific discussion closely, but in general, I wish designers put a higher value on consistency than on trying to guess what the audience's expectations are. Consistency leads to tools whose behavior can be reasoned about and competence with can be achieved. Guessing what the people want leads to incomprehensible black boxes that the user pokes at, hoping it will guess what they want it to do.

The specific situation under discussion is as follows:

1. Andrey has provided a function x.sub_and_test(v) which returns true when the result is zero. 2. Gavin argues that it should return true when the result is not zero. 3. I say that the function should be x.sub_and_test_if_zero(v) which returns true when the result is zero.

In (1), Andrey has provided the function as it is not because this is his guess of what people want, but because true on zero feels more natural as it's much more common to test for zero than to test for nonzero after subtraction. (Not so after a bitwise operation, admittedly.)

I don't have the OP handy, but I thought Andrey said it was because that matched the expression he created the function to replace.

In (2), Gavin argues for true on nonzero not because this is what people want, but because of consistency.

In (3), I suggest the rename not because this is what people want, but because I want to remove the ambiguity and the need to guess.

I argue that consistency with the other functions is enough to solve this problem since any user of the library must learn its conventions. The rest of the functions are bound to confuse a subset of users, but the greater confusion comes from the function in question testing for a different but value than the rest. To follow your approach would, I contend, require renaming the other functions in a similar manner. I'm not against that idea, but it is a bigger change. -- Rob (Sent from my portable computation device.)

Mere moments ago, quoth I:

On 26/01/2018 12:03, Peter Dimov wrote:

...

This is similar to the bool conversion of std::error_code, which is also perfectly consistent and still manages to return the opposite of what a certain fraction of the audience intuitively expects.

Sorry, hit Send too soon on the prior reply. That's a different case; it's taking the "true if non-zero" convention (the current implementation, based on the fact that internally it's an integer-like thing) and the "true if non-empty" that is instead implemented by things like smart pointers and optional (for similar reasons), and then conflating it with an assumption that "empty == success" given that the class name is error_code. Which is usually but not always true. The proposed change is to address that assumption by treating it less like a number and more like an error state. But incidentally, error_code's current implementation also goes with the idea that non-zero is true, so it supports my case. ;)

On 26/01/2018 01:45, Andrey Semashev wrote:

So would you prefer to keep the name and change the result?

That is my preference, yes. Though I know it could be problematic since it's an effectively invisible breaking change. And while it's new, there could still be some people using it in its current form before 1.67 is released. Deprecating the current method and adding a new one with a new name is the safer way to change the behaviour, but unfortunately "x_and_test" already seems like the best name for this operation. "add_test" could also work I guess (since existing methods like "fetch_add" also lack a conjunction, I suppose one could argue that it could be even more consistent that way). And it does get rid of the slightly awkward "and_and_test", so it's not all bad.

On 01/26/18 03:08, Peter Dimov via Boost wrote:

...

    if (a.sub_and_test(1))

Also, the generated code for these operations can potentially be more efficient.

While we're on the subject, on what architectures would opaque_sub be more efficient than sub_and_test?

On x86 and gcc < 7 opaque_sub allows to use "lock sub" or "lock dec" without setting the bool according to the zero flag, i.e. it saves a register and an instruction. Gcc 7 introduced the ability to return flags from the asm statement, so the code can be written the same way. Although I noticed that the compiler tends to save the flag into a register early unless it is tested immediately, so in some cases opaque_sub might still be preferable where it suits. AFAIK, other compilers, including clang, don't support this feature.

On 01/26/18 16:32, Peter Dimov via Boost wrote:

Andrey Semashev wrote:

...

...

While we're on the subject, on what architectures would opaque_sub be > more efficient than sub_and_test?

On x86 and gcc < 7 opaque_sub allows to use "lock sub" or "lock dec" without setting the bool according to the zero flag, i.e. it saves a register and an instruction.

Right, thanks. I was thinking that testing for zero comes for free, but it's not (entirely) free for the reason you give. Does this actually matter in practice? I would expect the atomic to dominate the `set(n)z al`.

Latency-wise, I expect that to be mostly true. But wasting a register may be undesirable if it causes a spill on the stack somewhere in the surrounding code, especially if this is a tight loop. In any case, I just want to be able to generate the best possible code with the interface atomic<> provides.

...

Gcc 7 introduced the ability to return flags from the asm statement, so the code can be written the same way. Although I noticed that the compiler tends to save the flag into a register early unless it is tested immediately, so in some cases opaque_sub might still be preferable where it suits.

Don't see how opaque_sub could be preferable if you need to test the flag later. :-)

Of course. :) I meant, in the case where you don't need the result, opaque_sub is still preferable to fetch_sub or sub_and_test.

Presumably, if you just call the function and discard the return value - the equivalent of opaque_ - the compiler would be smart enough to not save the flag.

Hopefully, but I wouldn't bet on it. I've seen gcc generate "setz" then a couple of "movs" which were moved from god knows where and then "test" and a conditional jump. Clearly, "movs" don't alter flags, so the spill and the test are useless. Admittedly, dropping "setz" when the result is unused is a different kind of optimization. But my point is that optimizations like these are generally unreliable, and if you really want to have the best possible code then you should better write it in a way so the compiler has less opportunity to screw up.

I remember some compilers being smart enough to notice that you don't use the result of the atomic fetch_op intrinsic and generating the `lock op` themselves, without a separate opaque_op being needed. We can't do that on the library level, of course.

Yes, I've seen gcc 7 (and maybe 6?) do that on occasion, but it seemed that it didn't always do that either. I didn't investigate that closely to find out why it didn't always optimize.

On 01/26/18 18:20, Peter Dimov via Boost wrote:

Andrey Semashev wrote:

...

But my point is that optimizations like these are generally unreliable, and if you really want to have the best possible code then you should better write it in a way so the compiler has less opportunity to screw up.

If we adopt this philosophy, shouldn't there be `opaque_` and `_and_test` increments and decrements, too? `lock inc [r]` instead of `lock add [r], 1`, or even `mov eax, 1; lock add [r], eax` if the compiler can't be trusted to optimize, which we assume.

To some extent, I'm already doing this: https://github.com/boostorg/atomic/blob/develop/include/boost/atomic/detail/... In general though, there has to be a reasonable balance between the library usability and efficiency. I think the current interface is quite close to it.

On 01/26/18 19:32, Peter Dimov via Boost wrote:

The interface strikes me as heavily influenced by what g++ can (__builtin_constant_p) and cannot (figure out that the result is not needed) do. The order of adding the functions probably also plays a part; were `op_and_test` added first, `opaque_op` probably wouldn't have been.

No, all extra ops were added at the same time. I was also planning to add a generalized `read_modify_write` operation but didn't do it because I don't have the hardware with TSX. It's true though that my main testing compiler is gcc. Clang doesn't seem to support __builtin_constant_p, which makes it fail to convert "add" to "inc" in add_and_test and opaque_add. OTOH, Intel compiler does support it and generates better code for add_and_test and opaque_add than for fetch_add. Maybe I should switch clang to the generic emulation backend. If the code can be improved for other compilers, I welcome suggestions and patches.

It's interesting to play and see what gets generated when. For instance, clang++ 3.6 figures out by itself that in `x1.fetch_and_add( 1 );` the result is not used, and generates `lock inc`.

https://godbolt.org/g/7TFQ2V

How does it manage to do that, if you're using assembly `xadd`, I don't know.

`fetch_add` is implemented in terms of instrinsics (I assume, clang supports __atomic* intrinsics, so those should be used). It is expected that there is no difference to `std::atomic` in the standard operations on recent compilers.

Andrey Semashev wrote:

Clang doesn't seem to support __builtin_constant_p, which makes it fail to convert "add" to "inc" in add_and_test and opaque_add. Maybe I should switch clang to the generic emulation backend.

This would make it use `xadd` instead of `add`, see https://godbolt.org/g/V1iTbj. Adding `inc_and_test` seems more straightforward and consistent with the spirit of the current interface that lets the user express the exact operation instead of relying on the compiler to figure it out.

OTOH, Intel compiler does support it and generates better code for add_and_test and opaque_add than for fetch_add.

Indeed it does, https://godbolt.org/g/QAbTZo. I checked the g++ and clang++ versions and was going to say that nowhere does opaque_add generate better code than the dumb fetch_add, but it does on Intel.

`fetch_add` is implemented in terms of instrinsics (I assume, clang supports __atomic* intrinsics, so those should be used).

Ah right. To get back on topic, I'm fine with making op_and_test return true on nonzero.