Hi Sanjin,<br><br>Are you sure we don't need the uniqueness property of a set any more?<br><br>If not, SetVector seems like the right solution. <br><br>James<br><div class="gmail_quote">On Wed, 28 Jan 2015 at 18:48, Sanjin Sijaric <<a href="mailto:ssijaric@codeaurora.org">ssijaric@codeaurora.org</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi jmolloy, Jiangning, t.p.northover,<br>
<br>
Change the type of AllChains from std::set<unique_ptr<Chain>> to std::vector<unique_ptr<Chain>><u></u>, 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.<br>
<br>
Example debug output with -debug-only=aarch64-a57-fp-<u></u>load-balancing resulting in different traversal:<br>
<br>
Compile #1:<br>
<br>
colorChainSet(): #sets=4<br>
- Parity=2, Color=Odd<br>
- colorChain({%S19<def> = FMULSrr -> %S19<def> = FMULSrr}, Odd)<br>
- Scavenged register: S3<br>
- Destination register not changed.<br>
- Parity=1, Color=Odd<br>
- {%S18<def> = FMULSrr -> %S18<def> = FMULSrr} - not worthwhile changing; color remains Even<br>
- colorChain({%S18<def> = FMULSrr -> %S18<def> = FMULSrr}, Even)<br>
- Scavenged register: S2<br>
- Destination register not changed.<br>
- Parity=2, Color=Odd<br>
- colorChain({%S6<def> = FMULSrr -> %S6<def> = FMULSrr (kill @ %S18<def> = FMULSrr)}, Odd)<br>
- Scavenged register: S3<br>
- Kill instruction seen.<br>
- Parity=1, Color=Odd<br>
- colorChain({%S16<def> = FMULSrr -> %S16<def> = FMULSrr (kill @ %S19<def> = FMULSrr)}, Odd)<br>
- Scavenged register: S5<br>
- Kill instruction seen.<br>
<br>
Compile #2:<br>
<br>
colorChainSet(): #sets=4<br>
- Parity=2, Color=Odd<br>
- colorChain({%S19<def> = FMULSrr -> %S19<def> = FMULSrr}, Odd)<br>
- Scavenged register: S3<br>
- Destination register not changed.<br>
- Parity=1, Color=Odd<br>
- {%S18<def> = FMULSrr -> %S18<def> = FMULSrr} - not worthwhile changing; color remains Even<br>
- colorChain({%S18<def> = FMULSrr -> %S18<def> = FMULSrr}, Even)<br>
- Scavenged register: S2<br>
- Destination register not changed.<br>
- Parity=2, Color=Odd<br>
- colorChain({%S16<def> = FMULSrr -> %S16<def> = FMULSrr (kill @ %S19<def> = FMULSrr)}, Odd)<br>
- Scavenged register: S3<br>
- Kill instruction seen.<br>
- Parity=1, Color=Odd<br>
- colorChain({%S6<def> = FMULSrr -> %S6<def> = FMULSrr (kill @ %S18<def> = FMULSrr)}, Odd)<br>
- Scavenged register: S5<br>
- Kill instruction seen.<br>
<br>
Cannot have a reproducible test case for this.<br>
<br>
<a href="http://reviews.llvm.org/D7231" target="_blank">http://reviews.llvm.org/D7231</a><br>
<br>
Files:<br>
lib/Target/AArch64/<u></u>AArch64A57FPLoadBalancing.cpp<br>
<br>
Index: lib/Target/AArch64/<u></u>AArch64A57FPLoadBalancing.cpp<br>
==============================<u></u>==============================<u></u>=======<br>
--- lib/Target/AArch64/<u></u>AArch64A57FPLoadBalancing.cpp<br>
+++ lib/Target/AArch64/<u></u>AArch64A57FPLoadBalancing.cpp<br>
@@ -137,7 +137,7 @@<br>
int scavengeRegister(Chain *G, Color C, MachineBasicBlock &MBB);<br>
void scanInstruction(MachineInstr *MI, unsigned Idx,<br>
std::map<unsigned, Chain*> &Active,<br>
- std::set<std::unique_ptr<<u></u>Chain>> &AllChains);<br>
+ std::vector<std::unique_ptr<<u></u>Chain>> &AllChains);<br>
void maybeKillChain(MachineOperand &MO, unsigned Idx,<br>
std::map<unsigned, Chain*> &RegChains);<br>
Color getColor(unsigned Register);<br>
@@ -320,7 +320,7 @@<br>
// been killed yet. This is keyed by register - all chains can only have one<br>
// "link" register between each inst in the chain.<br>
std::map<unsigned, Chain*> ActiveChains;<br>
- std::set<std::unique_ptr<<u></u>Chain>> AllChains;<br>
+ std::vector<std::unique_ptr<<u></u>Chain>> AllChains;<br>
unsigned Idx = 0;<br>
for (auto &MI : MBB)<br>
scanInstruction(&MI, Idx++, ActiveChains, AllChains);<br>
@@ -583,7 +583,7 @@<br>
void AArch64A57FPLoadBalancing::<br>
scanInstruction(MachineInstr *MI, unsigned Idx,<br>
std::map<unsigned, Chain*> &ActiveChains,<br>
- std::set<std::unique_ptr<<u></u>Chain>> &AllChains) {<br>
+ std::vector<std::unique_ptr<<u></u>Chain>> &AllChains) {<br>
// Inspect "MI", updating ActiveChains and AllChains.<br>
<br>
if (isMul(MI)) {<br>
@@ -602,7 +602,7 @@<br>
<br>
auto G = llvm::make_unique<Chain>(MI, Idx, getColor(DestReg));<br>
ActiveChains[DestReg] = G.get();<br>
- AllChains.insert(std::move(G))<u></u>;<br>
+ AllChains.push_back(std::move(<u></u>G));<br>
<br>
} else if (isMla(MI)) {<br>
<br>
@@ -646,7 +646,7 @@<br>
<< TRI->getName(DestReg) << "\n");<br>
auto G = llvm::make_unique<Chain>(MI, Idx, getColor(DestReg));<br>
ActiveChains[DestReg] = G.get();<br>
- AllChains.insert(std::move(G))<u></u>;<br>
+ AllChains.push_back(std::move(<u></u>G));<br>
<br>
} else {<br>
<br>
EMAIL PREFERENCES<br>
<a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/<u></u>settings/panel/<u></u>emailpreferences/</a><br>
______________________________<u></u>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/llvm-commits</a><br>
</blockquote></div>