Page MenuHomePhabricator

AArch64: Make AArch64A57FPLoadBalancing output stable.
ClosedPublic

Authored by gberry on Jan 29 2015, 2:15 PM.

Details

Summary

Add tie breaker to colorChainSet() sort so that processing order doesn't
depend on std::set order, which depends on pointer order, which is
unstable from run to run.

Diff Detail

Event Timeline

gberry updated this revision to Diff 18986.Jan 29 2015, 2:15 PM
gberry retitled this revision from to AArch64: Make AArch64A57FPLoadBalancing output stable..
gberry updated this object.
gberry edited the test plan for this revision. (Show Details)
gberry added reviewers: jmolloy, mcrosier, apazos.
gberry set the repository for this revision to rL LLVM.
gberry added a subscriber: Unknown Object (MLST).

Hi James,

No, I don’t think just switching to std::stable_sort() would solve the problem. The elements that are “equal” based on the sort comparison would remain in their original order before the sort, which I believe is determined by the order they occur in the EquivalenceClasses member iterator, which is in turn determined by pointer order since the EquivalenceClasses members are stored in a set<Chain*> in this case.

Hopefully that makes sense. If so, could you commit this change?

Thanks,

-Geoff

Geoff Berry

Employee of Qualcomm Innovation Center, Inc.

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

From: James Molloy [mailto:james@jamesmolloy.co.uk]
Sent: Friday, January 30, 2015 3:41 AM
To: reviews+D7265+public+9863efb7948c8628@reviews.llvm.org; gberry@codeaurora.org; james.molloy@arm.com; mcrosier@codeaurora.org; apazos@codeaurora.org
Cc: llvm-commits@cs.uiuc.edu; kanheim@a-bix.com
Subject: Re: [PATCH] AArch64: Make AArch64A57FPLoadBalancing output stable.

Hi Geoff,

Thanks for fixing this! It looks ok as-is, but wouldn't switching simply from std::sort to std::stable_sort solve the problem easier? Stable_sort is used for this reason in other areas of the compiler.

Cheers,

James

bmakam added a subscriber: bmakam.Jan 30 2015, 8:14 AM

Adding Rafael to the review, since he had fixed similar issues in the past (r196520) relating to non-deterministic behavior.
Rafael, do you think this is a good fix to the non-deterministic behavior we are seeing?

mcrosier accepted this revision.Jan 30 2015, 11:57 AM
mcrosier edited edge metadata.

Based on James' comments and Geoff's feedback, I'm going to go ahead and approve the change.

This revision is now accepted and ready to land.Jan 30 2015, 11:57 AM
mcrosier closed this revision.Jan 30 2015, 11:58 AM

Committed in r227606.