[PATCH] D39947: [OpenMP] Stable sort Privates to remove non-deterministic ordering

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D39947#922922, @mgrang wrote:

> In https://reviews.llvm.org/D39947#922889, @rjmccall wrote:
>
> > In https://reviews.llvm.org/D39947#922870, @mgrang wrote:
> >
> > > Although this patches fixes the above unit test failures, the generated code is very different from the one that the tests expect. As a result, these tests need to be adjusted. Could the reviewers please comment/suggest on whether it is ok to fix the tests as a result of this change?
> > >
> > > The other way of obtaining deterministic ordering for privates with the same alignment is to use an index for each item inserted into Privates and use it as a tie-breaker. But even in that case the generated code is quite different and tests still need to be adjusted.
> >
> >
> > Fixing the tests may be acceptable.  Can you give an example of the difference between the old and new test outputs?
>
>
> Please see https://www.diffchecker.com/7V2XFbk4 for the difference in output for the following test before and after my change:
>
>   clang -cc1 -internal-isystem <MYDIR>/build/llvm/lib/clang/6.0.0/include -nostdsysteminc -verify -fopenmp -x c++ -triple x86_64-apple-darwin10 -emit-llvm <MYDIR>/src/llvm/tools/clang/test/OpenMP/task_firstprivate_codegen.cpp -o -
>


Does your diff have shuffling enabled on both sides?  Neither layout for %struct..kmp_privates.t.3 seems to match the test's match for PRIVATES_TMAIN_TY, so I'm not completely sure which is supposed to be which.  Assuming that the right diff is with your patch, something seems quite wrong, because the capture for t_var is being sorted to the end, which is producing a really terrible layout.

I think you might actually have accidentally inverted the order: a qsort comparator is supposed to return positive if ``LHS > RHS``, so the fact that it's returning 1 when ``P1->first < P2->first`` means that it's actually a reversed comparison.  Would you mind fixing that and then letting us know what test changes remain?

Cou


https://reviews.llvm.org/D39947