[PATCH] [AArch64] Fix non-determinism in A57 FP Load Balancing

[Sanjin Sijaric]

Hi jmolloy, Jiangning, t.p.northover,

Change the type of AllChains from std::set<unique_ptr<Chain>> to std::vector<unique_ptr<Chain>>, as traversing pointers contained in std::set doesn't guarantee a well-defined order.  We have observed differences in register assignment in .s files produced with the exact same build.  I don't see that duplicate entries can be added to AllChains, so it should be safe to replace std::set with std::vector.  This way, traversal of AllChains will be well defined.

Example debug output with -debug-only=aarch64-a57-fp-load-balancing resulting in different traversal:

Compile #1:

    colorChainSet(): #sets=4
   - Parity=2, Color=Odd
   - colorChain({%S19<def> = FMULSrr -> %S19<def> = FMULSrr}, Odd)
   - Scavenged register: S3
   - Destination register not changed.
   - Parity=1, Color=Odd
   - {%S18<def> = FMULSrr -> %S18<def> = FMULSrr} - not worthwhile changing; color remains Even
   - colorChain({%S18<def> = FMULSrr -> %S18<def> = FMULSrr}, Even)
   - Scavenged register: S2
   - Destination register not changed.
   - Parity=2, Color=Odd
   - colorChain({%S6<def> = FMULSrr -> %S6<def> = FMULSrr (kill @ %S18<def> = FMULSrr)}, Odd)
   - Scavenged register: S3
   - Kill instruction seen.
   - Parity=1, Color=Odd
   - colorChain({%S16<def> = FMULSrr -> %S16<def> = FMULSrr (kill @ %S19<def> = FMULSrr)}, Odd)
   - Scavenged register: S5
   - Kill instruction seen.

Compile #2:

  colorChainSet(): #sets=4
 - Parity=2, Color=Odd
 - colorChain({%S19<def> = FMULSrr -> %S19<def> = FMULSrr}, Odd)
 - Scavenged register: S3
 - Destination register not changed.
 - Parity=1, Color=Odd
 - {%S18<def> = FMULSrr -> %S18<def> = FMULSrr} - not worthwhile changing; color remains Even
 - colorChain({%S18<def> = FMULSrr -> %S18<def> = FMULSrr}, Even)
 - Scavenged register: S2
 - Destination register not changed.
 - Parity=2, Color=Odd
 - colorChain({%S16<def> = FMULSrr -> %S16<def> = FMULSrr (kill @ %S19<def> = FMULSrr)}, Odd)
 - Scavenged register: S3
 - Kill instruction seen.
 - Parity=1, Color=Odd
 - colorChain({%S6<def> = FMULSrr -> %S6<def> = FMULSrr (kill @ %S18<def> = FMULSrr)}, Odd)
 - Scavenged register: S5
 - Kill instruction seen.

Cannot have a reproducible test case for this.

http://reviews.llvm.org/D7231

Files:
  lib/Target/AArch64/AArch64A57FPLoadBalancing.cpp

Index: lib/Target/AArch64/AArch64A57FPLoadBalancing.cpp
===================================================================
--- lib/Target/AArch64/AArch64A57FPLoadBalancing.cpp
+++ lib/Target/AArch64/AArch64A57FPLoadBalancing.cpp
@@ -137,7 +137,7 @@
   int scavengeRegister(Chain *G, Color C, MachineBasicBlock &MBB);
   void scanInstruction(MachineInstr *MI, unsigned Idx,
                        std::map<unsigned, Chain*> &Active,
-                       std::set<std::unique_ptr<Chain>> &AllChains);
+                       std::vector<std::unique_ptr<Chain>> &AllChains);
   void maybeKillChain(MachineOperand &MO, unsigned Idx,
                       std::map<unsigned, Chain*> &RegChains);
   Color getColor(unsigned Register);
@@ -320,7 +320,7 @@
   // been killed yet. This is keyed by register - all chains can only have one
   // "link" register between each inst in the chain.
   std::map<unsigned, Chain*> ActiveChains;
-  std::set<std::unique_ptr<Chain>> AllChains;
+  std::vector<std::unique_ptr<Chain>> AllChains;
   unsigned Idx = 0;
   for (auto &MI : MBB)
     scanInstruction(&MI, Idx++, ActiveChains, AllChains);
@@ -583,7 +583,7 @@
 void AArch64A57FPLoadBalancing::
 scanInstruction(MachineInstr *MI, unsigned Idx, 
                 std::map<unsigned, Chain*> &ActiveChains,
-                std::set<std::unique_ptr<Chain>> &AllChains) {
+                std::vector<std::unique_ptr<Chain>> &AllChains) {
   // Inspect "MI", updating ActiveChains and AllChains.
 
   if (isMul(MI)) {
@@ -602,7 +602,7 @@
 
     auto G = llvm::make_unique<Chain>(MI, Idx, getColor(DestReg));
     ActiveChains[DestReg] = G.get();
-    AllChains.insert(std::move(G));
+    AllChains.push_back(std::move(G));
 
   } else if (isMla(MI)) {
 
@@ -646,7 +646,7 @@
           << TRI->getName(DestReg) << "\n");
     auto G = llvm::make_unique<Chain>(MI, Idx, getColor(DestReg));
     ActiveChains[DestReg] = G.get();
-    AllChains.insert(std::move(G));
+    AllChains.push_back(std::move(G));
 
   } else {

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D7231.18903.patch
Type: text/x-patch
Size: 2007 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150128/eb36a872/attachment.bin>