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

[Vedant Kumar via llvm-commits]

> On Oct 24, 2017, at 4:43 PM, Grang, Mandeep Singh <mgrang at codeaurora.org> wrote:
> 
> 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?

In principle, I think a change like this is OK. Hopefully the inliner can do the right thing with the extra call. If a Release compiler with your change doesn't regress compile-time on LNT or CTMark substantially, it 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?

We've done wide-scale replacements for things like IWYU issues before. Assuming there's no substantial compile-time regression, I don't see how this is any different.

best,
vedant

> 
> 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
>> 
>> 
>> 
>