[PATCH] D39245: [ADT] Shuffle containers before sorting to uncover non-deterministic behavior

[Grang, Mandeep Singh via llvm-commits]

std::sort has these 4 overloads (from 
http://en.cppreference.com/w/cpp/algorithm/sort):

template< class RandomIt >
void sort( RandomIt first, RandomIt last );
(1)
template< class ExecutionPolicy, class RandomIt >
void sort( ExecutionPolicy&& policy, RandomIt first, RandomIt last );
(2)    (since C++17)
template< class RandomIt, class Compare >
void sort( RandomIt first, RandomIt last, Compare comp );
(3)
template< class ExecutionPolicy, class RandomIt, class Compare >
void sort( ExecutionPolicy&& policy, RandomIt first, RandomIt last, 
Compare comp );
(4)    (since C++17)

We would need to add a corresponding llvm::sort for each of these.

> I think the best thing to do is to update the callers to call `llvm::sort` instead of `std::sort`.
I wasn't sure if the community would accept such a wide-scale change. Each call to std::sort would then mean an extra function call. I guess that should be OK?

Should we let the owners change their respective codes to use llvm::sort instead of std::sort or submit a single patch (one each for llvm/clang/polly/compiler-rt) which does a wide-scale replace?

On 10/24/2017 4:32 PM, Duncan P. N. Exon Smith via Phabricator wrote:
> dexonsmith added a comment.
>
> In https://reviews.llvm.org/D39245#905862, @mgrang wrote:
>
>> In https://reviews.llvm.org/D39245#905733, @dexonsmith wrote:
>>
>>>> std::sort and array_pod_sort both use quicksort
>>> FWIW, most standards-compliant std::sort implementations use introsort... although it's also unstable.
>>>
>>>> NOTE: To randomly shuffle before std::sort we may have to change all calls to std::sort with llvm::sort.
>>> It looks to me like the current overloads of llvm::sort take a parallel execution policy, which would be unfortunate boilerplate to add.  Perhaps you should create an overload in ADT/STLExtras.h with the same signature as std::sort (that wraps std::sort with the initial call to std::random_shuffle).
>>
>> Sure, I can add overload for llvm::sort which takes ExecutionPolicy and invoke std::sort.
>
> You mean, add an overload that does not take ExecutionPolicy?
>
>> However, it would still mean changing all calls to std::sort with llvm::sort if this feature needs to be tested. Is there a way to do this w/o changing the callers? #define std::sort llvm::sort won't work because of the double colon (::), redefining std::sort will result in duplicate definitions, etc.
> I think the best thing to do is to update the callers to call `llvm::sort` instead of `std::sort`.
>
>
> Repository:
>    rL LLVM
>
> https://reviews.llvm.org/D39245
>
>
>