[PATCH] D15302: [Greedy regalloc] Replace analyzeSiblingValues with something new [Part1]
Quentin Colombet via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 21 12:22:05 PDT 2016
qcolombet added a comment.
Hi Wei,
I believe we are almost done. Thanks for your work and patience on this.
There are mainly three items to address:
- There are typos widely spread in the file; mergable -> mergeable, redundent -> redundant
- Do not repeat method names on comment.
- Fix on the test cases. See the inline comment.
As for the benchmarking, almost all the diffs came back as improvement of up to 7%! This is impressive.
The regressions seem like side effect, i.e., we generate less load pair in a few case, because the related spill slots are not next to each other anymore. This was luck previously.
Anyhow, looking forward for the final fix-ups.
Cheers,
-Quentin
================
Comment at: include/llvm/CodeGen/LiveRangeEdit.h:77
@@ +76,3 @@
+ /// rematerialization but not deleted yet -- to be done in postOptimization.
+ SmallPtrSet<MachineInstr *, 32> *DeadRemats;
+
----------------
Switch to SmallPtrSetImpl, this the size of the type is not relevant.
================
Comment at: include/llvm/CodeGen/LiveRangeEdit.h:152
@@ -144,1 +151,3 @@
+ /// pop_back - It allows LiveRangeEdit users to drop new registers.
+ /// The context is when an original def instruction of a register is
----------------
Don’t repeat the method name in the comment.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:76
@@ +75,3 @@
+ typedef DenseMap<std::pair<int, VNInfo *>, SmallPtrSet<MachineInstr *, 16>>
+ MergableSpillsMap;
+ MergableSpillsMap MergableSpills;
----------------
Mergeable.
Do a search, the typo is widely spread :).
================
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:
----------------
Don’t repeat the method name.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:1040
@@ +1039,3 @@
+
+/// postOptimization - Optimizations after all the reg selections and spills
+/// are done.
----------------
Don’t repeat the name of the method.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:1051
@@ +1050,3 @@
+
+/// addToMergableSpills - When a spill is inserted, add the spill to
+/// MergableSpills map.
----------------
More mergeable typos...
================
Comment at: lib/CodeGen/LiveRangeEdit.cpp:382
@@ +381,3 @@
+ if (NoSplit)
+ continue;
+
----------------
Some update problem I believe.
================
Comment at: lib/CodeGen/RegAllocBase.cpp:159
@@ +158,3 @@
+ spiller().postOptimization();
+ for (auto ent : DeadRemats) {
+ LIS->RemoveMachineInstrFromMaps(*ent);
----------------
Capitale letter for the first letter of the variable name.
================
Comment at: lib/CodeGen/RegAllocBase.h:88
@@ +87,3 @@
+ // rematerialization.
+ void postOptimization();
+
----------------
Add virtual keyword.
Subclasses may want to do additional things.
================
Comment at: lib/CodeGen/RegAllocPBQP.cpp:727
@@ +726,3 @@
+ /// Remove dead defs because of rematerialization.
+ for (auto ent : DeadRemats) {
+ LIS.RemoveMachineInstrFromMaps(*ent);
----------------
Variables start with a capital letter.
================
Comment at: lib/CodeGen/Spiller.h:32
@@ -30,3 +31,3 @@
virtual void spill(LiveRangeEdit &LRE) = 0;
-
+ virtual void postOptimization() = 0;
};
----------------
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.
================
Comment at: lib/CodeGen/Spiller.h:39
@@ -37,4 +38,3 @@
MachineFunction &mf,
VirtRegMap &vrm);
}
----------------
I was thinking in case we want to test without the post-optimization.
But I am fine if it is always enabled.
================
Comment at: lib/CodeGen/SplitKit.h:335
@@ +334,3 @@
+ /// others.
+ void computeRedundentBackCopies(DenseSet<unsigned> &NotToHoistSet,
+ SmallVectorImpl<VNInfo *> &BackCopies);
----------------
Typo: redundant
================
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.
----------------
Make this a file check test.
================
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" }
----------------
Get rid of the attributes if they are not actually needed.
================
Comment at: test/CodeGen/X86/new-remat.ll:12
@@ +11,3 @@
+; Function Attrs: nounwind uwtable
+define i32 @uniform_testdata(i32 %p1) #0 {
+entry:
----------------
Use opt -instnamer to get rid of the %[0-9]+ variables.
Repository:
rL LLVM
http://reviews.llvm.org/D15302
More information about the llvm-commits
mailing list