[PATCH] D15302: [Greedy regalloc] Replace analyzeSiblingValues with something new [Part1]

[Wei Mi via llvm-commits]

wmi added inline comments.

================
Comment at: lib/CodeGen/InlineSpiller.cpp:76
@@ +75,3 @@
+  typedef DenseMap<std::pair<int, VNInfo *>, SmallPtrSet<MachineInstr *, 16>>
+      MergableSpillsMap;
+  MergableSpillsMap MergableSpills;
----------------
qcolombet wrote:
> Mergeable.
> Do a search, the typo is widely spread :).
Fixed.

================
Comment at: lib/CodeGen/InlineSpiller.cpp:326
@@ -317,28 +325,3 @@
 
-#ifndef NDEBUG
-static raw_ostream &operator<<(raw_ostream &OS,
-                               const InlineSpiller::SibValueInfo &SVI) {
-  OS << "spill " << PrintReg(SVI.SpillReg) << ':'
-     << SVI.SpillVNI->id << '@' << SVI.SpillVNI->def;
-  if (SVI.SpillMBB)
-    OS << " in BB#" << SVI.SpillMBB->getNumber();
-  if (SVI.AllDefsAreReloads)
-    OS << " all-reloads";
-  if (SVI.DefByOrigPHI)
-    OS << " orig-phi";
-  if (SVI.KillsSource)
-    OS << " kill";
-  OS << " deps[";
-  for (VNInfo *Dep : SVI.Deps)
-    OS << ' ' << Dep->id << '@' << Dep->def;
-  OS << " ]";
-  if (SVI.DefMI)
-    OS << " def: " << *SVI.DefMI;
-  else
-    OS << '\n';
-  return OS;
-}
-#endif
-
-/// propagateSiblingValue - Propagate the value in SVI to dependents if it is
-/// known.  Otherwise remember the dependency for later.
+/// hoistSpillInsideBB - It is beneficial to spill to earlier place in the
+/// same BB in case as follows:
----------------
qcolombet wrote:
> Don’t repeat the method name.
Fixed.

================
Comment at: lib/CodeGen/InlineSpiller.cpp:1040
@@ +1039,3 @@
+
+/// postOptimization - Optimizations after all the reg selections and spills
+/// are done.
----------------
qcolombet wrote:
> Don’t repeat the name of the method.
All similar comments fixed.

================
Comment at: lib/CodeGen/InlineSpiller.cpp:1051
@@ +1050,3 @@
+
+/// addToMergableSpills - When a spill is inserted, add the spill to
+/// MergableSpills map.
----------------
qcolombet wrote:
> More mergeable typos...
All such typos Fixed.

================
Comment at: lib/CodeGen/LiveRangeEdit.cpp:382
@@ +381,3 @@
+    if (NoSplit)
+      continue;
+
----------------
qcolombet wrote:
> Some update problem I believe.
Fixed.

================
Comment at: lib/CodeGen/RegAllocBase.cpp:159
@@ +158,3 @@
+  spiller().postOptimization();
+  for (auto ent : DeadRemats) {
+    LIS->RemoveMachineInstrFromMaps(*ent);
----------------
qcolombet wrote:
> Capitale letter for the first letter of the variable name.
Fixed.

================
Comment at: lib/CodeGen/RegAllocBase.h:88
@@ +87,3 @@
+  // rematerialization.
+  void postOptimization();
+
----------------
qcolombet wrote:
> Add virtual keyword.
> Subclasses may want to do additional things.
Fixed.

================
Comment at: lib/CodeGen/RegAllocPBQP.cpp:727
@@ +726,3 @@
+  /// Remove dead defs because of rematerialization.
+  for (auto ent : DeadRemats) {
+    LIS.RemoveMachineInstrFromMaps(*ent);
----------------
qcolombet wrote:
> Variables start with a capital letter.
Fixed.

================
Comment at: lib/CodeGen/Spiller.h:32
@@ -30,3 +31,3 @@
     virtual void spill(LiveRangeEdit &LRE) = 0;
-
+    virtual void postOptimization() = 0;
   };
----------------
qcolombet wrote:
> Other spillers out-of-tree may exist and there is little interest in having them to implement a post optimization method if they do not need it.
> In other words, instead of a pure virtual method, do nothing for the default implementation.
Make sense. Fixed.

================
Comment at: lib/CodeGen/Spiller.h:39
@@ -37,4 +38,3 @@
                                MachineFunction &mf,
                                VirtRegMap &vrm);
 }
----------------
qcolombet wrote:
> I was thinking in case we want to test without the post-optimization.
> But I am fine if it is always enabled.
Ok, I leave it there for now.

================
Comment at: lib/CodeGen/SplitKit.h:335
@@ +334,3 @@
+  /// others.
+  void computeRedundentBackCopies(DenseSet<unsigned> &NotToHoistSet,
+                                  SmallVectorImpl<VNInfo *> &BackCopies);
----------------
qcolombet wrote:
> Typo: redundant
Fixed here and many other places.

================
Comment at: test/CodeGen/X86/hoist-spill.ll:1
@@ +1,2 @@
+; RUN: llc < %s | grep 'Spill' |sed 's%.*\(-[0-9]\+(\%rsp)\).*%\1%g' |sort |uniq -d |awk '{if (/rsp/); exit -1}'
+; Check no spills to the same stack slot after hoisting.
----------------
qcolombet wrote:
> Make this a file check test.
I felt the file check test was not as general as the above test, but filecheck can still work, so I switch to file check here.  

================
Comment at: test/CodeGen/X86/hoist-spill.ll:115
@@ +114,2 @@
+
+attributes #0 = { norecurse noreturn nounwind uwtable "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2" "unsafe-fp-math"="false" "use-soft-float"="false" }
----------------
qcolombet wrote:
> Get rid of the attributes if they are not actually needed.
Fixed.

================
Comment at: test/CodeGen/X86/new-remat.ll:12
@@ +11,3 @@
+; Function Attrs: nounwind uwtable
+define i32 @uniform_testdata(i32 %p1) #0 {
+entry:
----------------
qcolombet wrote:
> Use opt -instnamer to get rid of the %[0-9]+ variables.
Fixed.


Repository:
  rL LLVM

http://reviews.llvm.org/D15302