[PATCH] D15302: [Greedy regalloc] Replace analyzeSiblingValues with something new [Part1]
Wei Mi via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 21 09:46:50 PDT 2016
wmi added inline comments.
================
Comment at: include/llvm/CodeGen/LiveRangeEdit.h:123
@@ -119,2 +122,3 @@
LiveRangeEdit(LiveInterval *parent, SmallVectorImpl<unsigned> &newRegs,
+ SmallPtrSet<MachineInstr *, 32> *deadRemats,
MachineFunction &MF, LiveIntervals &lis, VirtRegMap *vrm,
----------------
qcolombet wrote:
> Put that as at the end of the list with nullptr as default parameter.
Fixed.
================
Comment at: include/llvm/CodeGen/LiveRangeEdit.h:148
@@ -143,2 +147,3 @@
unsigned get(unsigned idx) const { return NewRegs[idx+FirstNew]; }
+ void pop_back() { NewRegs.pop_back(); }
----------------
I Added comments to explain it.
In short, we don't want to allocate phys register for the dummy register used as temporary dst register of instruction in DeadRemats set.
================
Comment at: include/llvm/CodeGen/LiveRangeEdit.h:184
@@ +183,3 @@
+ VNInfo *OrigVNI; // ParentVNI.def may be a copy only. OrigVNI.def
+ // contains the real expr for remat.
+ MachineInstr *OrigMI; // Instruction defining OrigVNI.
----------------
You are right. I don't have to save OrigVNI in struct Remat. Instead, add a parameter VNInfo *OrigVNI for LiveRangeEdit::canRematerializeAt.
================
Comment at: include/llvm/CodeGen/LiveRangeEdit.h:221
@@ +220,3 @@
+ // For regallocs other than Greedy, DeadRemats is nullptr for now.
+ if (DeadRemats)
+ DeadRemats->insert(inst);
----------------
qcolombet wrote:
> Maybe just say that DeadRemats is an optional field.
> Mentioning Greedy here does not bring any value IMO.
Fixed.
================
Comment at: include/llvm/CodeGen/LiveRangeEdit.h:235-239
@@ -220,4 +234,7 @@
/// as currently those new intervals are not guaranteed to spill.
- void eliminateDeadDefs(SmallVectorImpl<MachineInstr*> &Dead,
- ArrayRef<unsigned> RegsBeingSpilled = None);
+ /// NoSplit indicates it is used after the iterations of selectOrSplit and
+ /// registers should not be split into new intervals.
+ void eliminateDeadDefs(SmallVectorImpl<MachineInstr *> &Dead,
+ ArrayRef<unsigned> RegsBeingSpilled = None,
+ bool NoSplit = false);
----------------
Fixed.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:79
@@ +78,3 @@
+
+ /// Virt2SibingsMap - This is the map from original register to a set
+ /// containing all its siblings. To hoist a spill to another BB, we need
----------------
qcolombet wrote:
> Do not repeat the name of the field in the comment.
Fixed
================
Comment at: lib/CodeGen/InlineSpiller.cpp:81
@@ +80,3 @@
+ /// containing all its siblings. To hoist a spill to another BB, we need
+ /// to find out a live sibling there and use it as the RHS of the new spill.
+ DenseMap<unsigned, DenseSet<unsigned>> Virt2SiblingsMap;
----------------
qcolombet wrote:
> […] as the source (instead of RHS) of the new ..
Fixed
================
Comment at: lib/CodeGen/InlineSpiller.cpp:82
@@ +81,3 @@
+ /// to find out a live sibling there and use it as the RHS of the new spill.
+ DenseMap<unsigned, DenseSet<unsigned>> Virt2SiblingsMap;
+
----------------
qcolombet wrote:
> How big are the sets?
> I would expect very few siblings on average and was wondering if a SmallSetVector or SmallSet would be more appropriate.
Most of the cases the size of it is less than 16 I guess, so I use SmallSetVector instead. I cannot use SmallSet because the set needs to be iterated.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:85
@@ +84,3 @@
+ bool isSpillCandBB(unsigned OrigReg, VNInfo *OrigVNI, MachineBasicBlock *BB,
+ unsigned &LiveReg);
+ void getVisitOrders(
----------------
qcolombet wrote:
> Please use reference for values that cannot be nullptr. I.e., OrigVNI and BB.
Fixed.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:90
@@ +89,3 @@
+ SmallVectorImpl<MachineInstr *> &SpillsToRm,
+ DenseMap<MachineDomTreeNode *, unsigned> &SpillsToKept,
+ DenseMap<MachineDomTreeNode *, MachineInstr *> &SpillBBToSpill);
----------------
qcolombet wrote:
> SpillsToKeep
Fixed.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:213
@@ +212,3 @@
+ HoistSpiller *HSpiller = new HoistSpiller(pass, mf, vrm);
+ (dyn_cast<InlineSpiller>(spiller))->setHSpiller(HSpiller);
+}
----------------
qcolombet wrote:
> Hid the instantiation of the hoist spiller helper in the inliner spiller.
> The positive side effect is that we won’t leak the memory!
Make HoistSpillHelper a field in inline spiller.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:223
@@ +222,3 @@
+ assert(NewVRegs.size() == 0 &&
+ "No new vregs should be generated in hoistAllSpills");
+}
----------------
qcolombet wrote:
> Hid the call to the hoist spiller helper in the inliner spiller.
I Added postOptimization as a pure virtual func in class Spiller, and put the code of hoist spiller helper inside of InlineSpiller::postOptimization.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:998
@@ +997,3 @@
+// .i.e, there should be a living sibling of OrigReg at the insert point.
+bool HoistSpiller::isSpillCandBB(unsigned OrigReg, VNInfo *OrigVNI,
+ MachineBasicBlock *BB, unsigned &LiveReg) {
----------------
qcolombet wrote:
> qcolombet wrote:
> > .i.e => i.e.
> Use doxygen style comment.
Fixed.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:998
@@ +997,3 @@
+// .i.e, there should be a living sibling of OrigReg at the insert point.
+bool HoistSpiller::isSpillCandBB(unsigned OrigReg, VNInfo *OrigVNI,
+ MachineBasicBlock *BB, unsigned &LiveReg) {
----------------
wmi wrote:
> qcolombet wrote:
> > qcolombet wrote:
> > > .i.e => i.e.
> > Use doxygen style comment.
> Fixed.
Fixed.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:1010
@@ +1009,3 @@
+
+ for (auto const ent : Siblings) {
+ LiveInterval &LI = LIS.getInterval(ent);
----------------
qcolombet wrote:
> DenseSet does not guarantee that the iteration order is stable from one run to another, does it?
>
> Although we should have several siblings live at the same time, this is theoretically possible.
> In other words, we should use a container that has a deterministic iteration order for reproducibility. See the earlier suggestion I made.
Yes, when turn on split-mode-size, after hoistCopies, it is possible that several siblings live at the same time. it is not just therotically possible but realistic.
I use SmallSetVector instead here.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:1024
@@ +1023,3 @@
+/// same time.
+void HoistSpiller::getVisitOrders(
+ MachineBasicBlock *Root, SmallPtrSet<MachineInstr *, 16> &Spills,
----------------
qcolombet wrote:
> qcolombet wrote:
> > This method would benefit a more verbose comment.
> > Maybe something along the line:
> > Starting from \p Root find a top-down traversal order of the dominator tree to visit all basic blocks containing the elements of \p Spills.
> > Redundent spills will be found and put into \p SpillsToRm at the same time.
> > \p SpillBBToSpill will populated as part of the process and maps a basic block to the first store occurring in the basic block.
> >
> > \post SpillToRm.union(Spills at post) == Spills at pre
> >
> > What is the usage of SpillsToKept? In particular the unsigned part?
> >
> > Should we consider to moving some of the arguments to field of the current hoistspill instance?
> This method does a bunch of things!
> Although I understand we want to share the logic that does the traversal and such, I found that it makes the code harder to read.
> I’d say as it is now and with more comment like I suggested, this is fine, but in general I rather have a better separation of concerns then try to optimize if it turns out to be problematic.
> I am guessing you already went through that process, we are just lacking the history :).
Thanks for your example comment. It is good. I copy most of them to the code.
> Should we consider to moving some of the arguments to field of the current hoistspill instance?
hoistSpillHelper now has the same lifetime as InlineSpiller instance, so the lifes of those arguments are much shorter than that. That is why I choose to keep them as func local objects.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:1024
@@ +1023,3 @@
+/// same time.
+void HoistSpiller::getVisitOrders(
+ MachineBasicBlock *Root, SmallPtrSet<MachineInstr *, 16> &Spills,
----------------
wmi wrote:
> qcolombet wrote:
> > qcolombet wrote:
> > > This method would benefit a more verbose comment.
> > > Maybe something along the line:
> > > Starting from \p Root find a top-down traversal order of the dominator tree to visit all basic blocks containing the elements of \p Spills.
> > > Redundent spills will be found and put into \p SpillsToRm at the same time.
> > > \p SpillBBToSpill will populated as part of the process and maps a basic block to the first store occurring in the basic block.
> > >
> > > \post SpillToRm.union(Spills at post) == Spills at pre
> > >
> > > What is the usage of SpillsToKept? In particular the unsigned part?
> > >
> > > Should we consider to moving some of the arguments to field of the current hoistspill instance?
> > This method does a bunch of things!
> > Although I understand we want to share the logic that does the traversal and such, I found that it makes the code harder to read.
> > I’d say as it is now and with more comment like I suggested, this is fine, but in general I rather have a better separation of concerns then try to optimize if it turns out to be problematic.
> > I am guessing you already went through that process, we are just lacking the history :).
> Thanks for your example comment. It is good. I copy most of them to the code.
>
> > Should we consider to moving some of the arguments to field of the current hoistspill instance?
> hoistSpillHelper now has the same lifetime as InlineSpiller instance, so the lifes of those arguments are much shorter than that. That is why I choose to keep them as func local objects.
>
>
After separate part of the work into HoistSpillHelper::rmRedundentSpills and add more comments in the func body, it may make the code easier to read now.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:1043
@@ +1042,3 @@
+ SpillsToRm.push_back(SpillToRm);
+ SpillBBToSpill[MDT.DT->getNode(Block)] = SpillToKeep;
+ } else {
----------------
qcolombet wrote:
> Any chance this could be updated when we insert the spill?
> Like I said, it just feels like getVisitOrders does too many things.
I separate the first part of the work to another func: HoistSpillHelper::rmRedundentSpills, so HoistSpiller::getVisitOrders is more focus on what its name describes.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:1051
@@ +1050,3 @@
+
+ SmallPtrSet<MachineDomTreeNode *, 8> WorkSet;
+ SmallPtrSet<MachineDomTreeNode *, 8> NodesOnPath;
----------------
qcolombet wrote:
> Please document what is WorkSet supposed to contain.
Done.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:1053
@@ +1052,3 @@
+ SmallPtrSet<MachineDomTreeNode *, 8> NodesOnPath;
+ MachineDomTreeNode *RootIDomNode = MDT[Root]->getIDom();
+ // For every node on the dominator tree with spill, walk upside on the
----------------
qcolombet wrote:
> I think we should describe what is the expected root, because it seems strange to me that we don’t just take the node for the Root.
Done.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:1065
@@ +1064,3 @@
+ while (Node != RootIDomNode) {
+ if (Node != MDT[Block] && SpillBBToSpill[Node]) {
+ SpillToRm = SpillBBToSpill[MDT[Block]];
----------------
qcolombet wrote:
> More comments, e.g.,
> Node dominates Block and already store the value.
> This store is redundant.
Done.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:1078
@@ +1077,3 @@
+ } else {
+ SpillsToKept[MDT[Block]] = 0;
+ WorkSet.insert(NodesOnPath.begin(), NodesOnPath.end());
----------------
qcolombet wrote:
> Ok, found the meaning of the unsigned elsewhere…
> A comment here as well would be great.
Done.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:1098
@@ +1097,3 @@
+ } while (idx != Orders.size());
+
+ DEBUG(dbgs() << "Orders size is " << Orders.size() << "\n");
----------------
qcolombet wrote:
> Assert Orders.size == WorkSet.size?
Done.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:1120
@@ +1119,3 @@
+ // during hoisting. If the spill to be inserted is an original spill
+ // (not a hoisted one), the value of the map entry is 0. If the spill
+ // is a hoisted spill, the value of the map entry is the VReg to be used
----------------
qcolombet wrote:
> We do not insert the original store, it is already there, right?
Yes, that is right.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:1135
@@ +1134,3 @@
+ SpillsInSubTree;
+ // Iterate Orders set in reverse order, which will be a bottom-top order
+ // in the dominator tree. Once we visit a dom tree node, we know its
----------------
qcolombet wrote:
> I believe we usually use bottom-up instead of bottom-top.
Fixed.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:1137
@@ +1136,3 @@
+ // in the dominator tree. Once we visit a dom tree node, we know its
+ // children has already been visited and the spill locations in the
+ // subtrees of all the children have been determined.
----------------
qcolombet wrote:
> have
Fixed.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:1174
@@ +1173,3 @@
+ for (const auto SpillBB : SpillsInSubTree[*RIt])
+ SpillCost += MBFI.getBlockFreq(SpillBB->getBlock());
+
----------------
qcolombet wrote:
> If the subtrees get big, we will end up recomputing this cost a bunch of time.
> Could it be something we keep alongside the subtree?
I keep it along side the subtree in SpillsInSubTreeMap.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:1181
@@ +1180,3 @@
+ : BranchProbability(1, 1);
+ if (SpillCost > MBFI.getBlockFreq(Block) * MarginProb) {
+ // Hoist: Move spills to current Block.
----------------
qcolombet wrote:
> We could add a mode for the hoist spiller, where code size is the priority. I.e., always hoist when SpillsInSubTree.size > 1
> A follow-up patch is fine.
Ok, I will do it in a follow-up patch.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:1208
@@ +1207,3 @@
+ }
+ // For spills in SpillsToKept with LiveReg set (.i.e, not original spill),
+ // save them to SpillsToIns.
----------------
qcolombet wrote:
> typo.
Fixed.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:1210
@@ +1209,3 @@
+ // save them to SpillsToIns.
+ for (const auto ent : SpillsToKept) {
+ if (ent.second)
----------------
qcolombet wrote:
> Variable must start with a capital letter.
> Also why use ent for the name?
Fixed.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:1218
@@ +1217,3 @@
+/// to a less hot spot.
+void HoistSpiller::hoistAllSpills(LiveRangeEdit &Edit) {
+ // Save the mapping between stackslot and its original reg.
----------------
qcolombet wrote:
> Explain the general algorithm here.
Done.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:1234
@@ +1233,3 @@
+ Virt2SiblingsMap[Original].insert(Reg);
+ }
+
----------------
qcolombet wrote:
> This loop scares me.
> Any chance this information can be built as we insert spill.
I simply remove the inner loop. SlotToOrigReg map will become somewhat bigger, but not a lot.
================
Comment at: lib/CodeGen/InlineSpiller.cpp:1242
@@ +1241,3 @@
+ SmallPtrSet<MachineInstr *, 16> &EqValSpills = ent.second;
+ if (!ent.second.size())
+ continue;
----------------
qcolombet wrote:
> empty
Fixed.
================
Comment at: lib/CodeGen/RegAllocGreedy.cpp:401
@@ -399,2 +400,3 @@
void tryHintsRecoloring();
+ void postOptimization();
----------------
qcolombet wrote:
> I would have put this into the base class.
Done.
================
Comment at: lib/CodeGen/RegAllocGreedy.cpp:2571
@@ +2570,3 @@
+ eliminateDeadRemats();
+ startHoistSpiller(*MF, *VRM, *LIS, &spiller());
+}
----------------
qcolombet wrote:
> spiller().postOptimization()
Done.
================
Comment at: lib/CodeGen/RegAllocGreedy.cpp:2597
@@ -2589,2 +2596,3 @@
SpillerInstance.reset(createInlineSpiller(*this, *MF, *VRM));
+ createHoistSpiller(*this, *MF, *VRM, &spiller());
Loops = &getAnalysis<MachineLoopInfo>();
----------------
qcolombet wrote:
> Should be created within the inline spiller. See my comment on createInlineSpiller.
Done.
================
Comment at: lib/CodeGen/RegAllocPBQP.cpp:130
@@ +129,3 @@
+ /// always available for the remat of all the siblings of the original reg.
+ SmallPtrSet<MachineInstr *, 32> DeadRemats;
+
----------------
qcolombet wrote:
> This should be a separate patch.
InlineSpiller is shared by all kinds of register allocator, so the DeadRemats logic is also needed by PBQP. If I separate the part out, I need to fix the related unit test.
================
Comment at: lib/CodeGen/RegAllocPBQP.cpp:731
@@ +730,3 @@
+ DeadRemats.clear();
+}
+
----------------
qcolombet wrote:
> We shouldn’t have to touch this.
My original comment that DeadRemats is non-empty only when the regalloc is Greedy is wrong. Actually, InlineSpiller and related Remat logic are shared by all register allocators.
And RegAllocPBQP is not a subclass of RegAllocBase, so the code is needed.
================
Comment at: lib/CodeGen/RegisterCoalescer.cpp:463
@@ -464,1 +462,3 @@
+ LiveRangeEdit(nullptr, NewRegs, nullptr, *MF, *LIS, nullptr, this)
+ .eliminateDeadDefs(DeadDefs);
}
----------------
qcolombet wrote:
> We shouldn’t have to touch this.
Fixed.
================
Comment at: lib/CodeGen/Spiller.h:38
@@ -37,3 +37,3 @@
MachineFunction &mf,
VirtRegMap &vrm);
----------------
qcolombet wrote:
> add a bool here that default to false for using a post optimization.
I don't get the intention to add a bool here. Is it used to guard post optimization? why it is needed?
================
Comment at: lib/CodeGen/Spiller.h:46
@@ -40,1 +45,3 @@
+ void startHoistSpiller(MachineFunction &mf, VirtRegMap &vrm,
+ LiveIntervals &lis, Spiller *);
}
----------------
qcolombet wrote:
> Get rid of those.
Done.
================
Comment at: lib/CodeGen/SplitKit.cpp:727
@@ +726,3 @@
+/// be hoisted because of too much cost.
+void SplitEditor::removeRedundentCopies(DenseSet<unsigned> &NotToHoistSet,
+ SmallVectorImpl<VNInfo *> &BackCopies) {
----------------
qcolombet wrote:
> Please don’t repeat the comment from the declaration.
Comment removed.
================
Comment at: lib/CodeGen/SplitKit.cpp:742
@@ +741,3 @@
+
+ // For VNI aggregation of each ParentVNI, collect dominated, .i.e,
+ // redundent VNIs to BackCopies.
----------------
qcolombet wrote:
> i.e.
Fixed.
================
Comment at: lib/CodeGen/SplitKit.cpp:749
@@ +748,3 @@
+ for (auto Ent1 : EqualVNs[ParentVNI->id]) {
+ for (auto Ent2 : EqualVNs[ParentVNI->id]) {
+ if (Ent1 == Ent2 || DominatedVNIs.count(Ent1) ||
----------------
qcolombet wrote:
> We should we start iterating with the next iterator instead of starting over.
> The next call to count should early continue the loop but still!
Fixed.
================
Comment at: lib/CodeGen/SplitKit.cpp:1134
@@ -1053,3 +1133,3 @@
case SM_Speed:
- llvm_unreachable("Spill mode 'speed' not implemented yet");
+ hoistCopies();
}
----------------
qcolombet wrote:
> Add a comment saying that hoistCopies will behave differently between size and speed, otherwise it feels like those modes are the same.
Added.
================
Comment at: lib/CodeGen/SplitKit.h:333
@@ -331,4 +332,3 @@
- /// hoistCopiesForSize - Hoist back-copies to the complement interval in a
- /// way that minimizes code size. This implements the SM_Size spill mode.
- void hoistCopiesForSize();
+ /// removeRedundentCopies - Remove redundent back-copies if it has been
+ /// decided those back-copies will not be hoisted.
----------------
qcolombet wrote:
> Don’t repeat the name of the method.
Fixed.
================
Comment at: lib/CodeGen/SplitKit.h:335
@@ +334,3 @@
+ /// decided those back-copies will not be hoisted.
+ void removeRedundentCopies(DenseSet<unsigned> &NotToHoistSet,
+ SmallVectorImpl<VNInfo *> &BackCopies);
----------------
qcolombet wrote:
> Given how this is used, the actual name of this method should be computeRedundentBackCopies.
Fixed.
================
Comment at: lib/CodeGen/SplitKit.h:338
@@ +337,3 @@
+
+ /// hoistCopies - Hoist back-copies to the complement interval.
+ void hoistCopies();
----------------
qcolombet wrote:
> Ditto.
Fixed.
Repository:
rL LLVM
http://reviews.llvm.org/D15302
More information about the llvm-commits
mailing list