Skip to content

Conversation

@AdamBucior
Copy link
Contributor

@AdamBucior AdamBucior commented Aug 7, 2020

A small PR to optimize (ranges::)(uninitialized_)fill(_n) when filling ranges of bool or (signed|unsigned)char(8_t) with other scalar types. Part of #431.

EDIT: Fixes #1183.

@AdamBucior AdamBucior requested a review from a team as a code owner August 7, 2020 14:02
bool output[] = {false, true, false};
fill(output, output + 3, 5);
for (bool elem : output) {
assert(elem == true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests will likely pass even if int is not properly converted to bool. I suggest

for (const bool& elem : output) {
    assert(memcmp(&elem, &true_value, sizeof(bool)) == 0);
}

Copy link
Contributor Author

@AdamBucior AdamBucior Aug 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On my PC the assertion fails when I run:

#include <cstring>
#include <cassert>

using namespace std;

int main() {
    bool b;
    memset(&b, 5, 1);
    assert(b == true);
}

And I can't use memcmp because it's not constexpr

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it depends on debug (/Od) vs release (/O2).
Maybe in debug == true is literally comparison against true, and in release it is just test and branch on flags.

If that's right, then since tests run in debug, this looks enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this code in release width /O2 (only replaced assert(b == true); with if (b != true) abort(); because assert does nothing in release) and it still fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It mustn't be UB because this code compiles (unless it's a compiler bug):

#include <bit>
#include <cassert>

using namespace std;

constexpr bool f() {
    char i = 5;
    return bit_cast<bool>(i);
}

int main() {
    constexpr bool b = f();
    static_assert(b != true);
    static_assert(b != false);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I found that this code compiles:

#include <bit>
#include <algorithm>

using namespace std;

constexpr bool f() {
    char i = 2;

    bool arr[]  = {false, true, true};
    bool arr2[] = {false, true, bit_cast<bool>(i)};
    return !equal(begin(arr), end(arr), begin(arr2));
}

int main() {
    static_assert(f());
}

// * They can't be bool. (Not even `bool == bool`; we're concerned about representations other than 0 and 1.)

Which means that equal should be able to use memcmp with bool.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting.
I think it is still UB (expected behavior is a possible UB manifestation)
Usually wrong bools act sometimes as true, but apparently constexpr evaluation does not trigger that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe assert(bit_cast<char>(elem) == bit_cast<char>(bool(true))) ?

Copy link
Contributor Author

@AdamBucior AdamBucior Aug 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe assert(bit_cast<char>(elem) == bit_cast<char>(bool(true))) ?

These tests are not C++20 only so bit_cast is not always available.

I think it is still UB (expected behavior is a possible UB manifestation)

And even if it's UB it means that there's no reason for equal not to use memcmp for bool - not using memcmp in this case would only make sense if bit_cast<bool>((char)2) == true was always true and we know now that at least sometimes it's not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also has_unique_object_representations_v<bool> is true which (I think) means that if two bools have different representations they must compare unequal.

@AdamBucior
Copy link
Contributor Author

AdamBucior commented Aug 7, 2020

Changed equal to allow for memcmp with bool (see thread above). memcmp should work well with bool == (all other sizeof 1 integral types)

@AdamBucior AdamBucior changed the title Use memset in a bit more cases Use memset and memcmp in a bit more cases Aug 7, 2020
@CaseyCarter CaseyCarter added the performance Must go faster label Aug 7, 2020
@AdamBucior
Copy link
Contributor Author

After doing some further investigation I discovered that comparing bool with unsigned char and char8_t seems to not be memcmp safe. I think memcmping bool with char and signed char is safe, but I'm not entirely sure and I can't really see anyone doing it so I decided to enable it for bool == bool only.

@AdamBucior
Copy link
Contributor Author

I think it's ready for review.

AdamBucior and others added 2 commits August 11, 2020 13:51
Co-authored-by: Alex Guteniev <gutenev@gmail.com>
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think this product change is good (I thought really hard about the metaprogramming and couldn't find any ways for it to fail), although I have one concern about warnings (or the lack thereof) and minor test nitpicks.

I think this will be ready for final review after a quick revision.

@miscco
Copy link
Contributor

miscco commented Sep 1, 2020

It seems your formatting is behaving strangely

@AdamBucior
Copy link
Contributor Author

It seems your formatting is behaving strangely

For some reason visual studio didn't run clang-format automatically.

@StephanTLavavej StephanTLavavej added the bug Something isn't working label Sep 2, 2020
@StephanTLavavej StephanTLavavej removed their assignment Sep 2, 2020
@CaseyCarter CaseyCarter self-assigned this Sep 4, 2020
@CaseyCarter CaseyCarter merged commit d6f32c8 into microsoft:master Sep 4, 2020
@CaseyCarter
Copy link
Contributor

Thanks for making our bitwise-equality optimizations a bit more effective!

@CaseyCarter CaseyCarter removed their assignment Sep 4, 2020
@AdamBucior AdamBucior deleted the fill-optimization branch September 4, 2020 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working performance Must go faster

Projects

None yet

Development

Successfully merging this pull request may close these issues.

<algorithm>: fill fails to compile with volatile pointer inputs

6 participants