[PATCH] D53994: Fixing lower bound regression in certain situations.

Denis Yaroshevskiy via Phabricator reviews at reviews.llvm.org
Wed Nov 14 05:49:12 PST 2018


dyaroshev added inline comments.


================
Comment at: include/algorithm:3231
     {
-        difference_type __l2 = __len / 2;
+        difference_type __l2 = _VSTD::__half_positive(__len);
         _ForwardIterator __m = __first;
----------------
ldionne wrote:
> I'll admit to being extremely surprised this makes any difference whatsoever. In my experience, Clang always emits the same instruction no matter what: `shr`. Adding @jfb to take a look. If he's OK with this, I'm OK with this.
That's a great idea, I was as surprised as you are.

I don't think you can just shr - you don't know if the distance is positive: (l - f) kinda doesn't have to be (though std::distance can assert that as a post condition).

Here are a few ways to find the middle: https://godbolt.org/z/csAbsx
signed, unsigned and right shift (this is what libstdc++ does: https://github.com/gcc-mirror/gcc/blob/58fddb25d239007c7a9bff0d0969322d9c1709a8/libstdc%2B%2B-v3/include/bits/stl_algobase.h#L972)

They all produce a different codegen.
The unsigned one was the fastest (slightly faster than >>) and I wasn't 100% sure that calling >> was OK, while the way this patch does it should be.


https://reviews.llvm.org/D53994





More information about the libcxx-commits mailing list