[PATCH] D32563: Add LiveRangeShrink pass to shrink live range within BB.
Matthias Braun via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue May 9 10:58:55 PDT 2017
MatzeB requested changes to this revision.
MatzeB added inline comments.
This revision now requires changes to proceed.
================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:10-12
+// This pass moves instructions close to the definition of its operands to
+// shrink live range of the def instruction. The code motion is limited within
+// the basic block.
----------------
Use `/// \file`, see http://llvm.org/docs/CodingStandards.html#file-headers
It would also be a good idea to add a sentence describing the rules/heuristic that decide what instructions get moved.
================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:34
+ void getAnalysisUsage(AnalysisUsage &AU) const override {
+ AU.setPreservesAll();
+ MachineFunctionPass::getAnalysisUsage(AU);
----------------
This seems like a bold statement for a pass that moves instructions around. I would only expect to see `AU.setPreservesCFG()` here.
================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:50
+namespace {
+typedef DenseMap<MachineInstr *, int> InstOrderMap;
+
----------------
You never seem to be using negative numbers, consider using `unsigned` instead of `int`.
================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:52
+
+// Returns \p New if it's dominated by \p Old, otherwise return \p Old.
+// \p M maintains a map from instruction to its dominating order that satisfies
----------------
We tend to use doxygen `///` to document functions.
================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:57-58
+// \p New.
+MachineInstr *FindDominatedInstruction(MachineInstr *New, MachineInstr *Old,
+ const InstOrderMap &M) {
+ assert(New != nullptr);
----------------
Use references for things that cannot be nullptr. That also saves you `assert(New != nullptr)`.
================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:64
+ return New;
+ int OrderOld = M.find(Old)->second;
+ int OrderNew = M.find(New)->second;
----------------
`M[Old]`?
================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:65
+ int OrderOld = M.find(Old)->second;
+ int OrderNew = M.find(New)->second;
+ if (OrderOld != OrderNew)
----------------
`M[New]` or better avoid the double lookup with `M.count(New)` above and use an M.find(New) for both.
================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:79
+// from instruction \p Start. Returns true if any store instruction is seen.
+bool BuildInstOrderMap(MachineBasicBlock::instr_iterator Start,
+ InstOrderMap &M) {
----------------
You probably want `MachineBasicBlock::iterator` here and in several other places as you do not plan to break bundles apart.
================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:84
+ int i = 0;
+ for (MachineInstr &I : make_range(Start, Start->getParent()->instr_end())) {
+ M[&I] = i++;
----------------
Why not pass a `MachineBasicBlock &MBB` as parameter to the function instead of the iterator and doing the `Start->getParent()->instr_end()` dance? You could have just used `for (MachineInstr &I : MBB) {}` in that case.
================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:93-94
+// Scan instructions following MI and collect any matching DBG_VALUEs.
+// FIXME: this is copied from MachineSink.cpp, need to refactor them into
+// utility function.
+void collectDebugValues(MachineInstr &MI,
----------------
Then why not do so in this patch.
================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:101
+
+ MachineBasicBlock::iterator DI = MI; ++DI;
+ for (MachineBasicBlock::iterator DE = MI.getParent()->end();
----------------
I'm surprised this compiles. In each case you should use `MI.getIterator()` for clarity. Could also use `DI = std::next(...)`
================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:102-103
+ MachineBasicBlock::iterator DI = MI; ++DI;
+ for (MachineBasicBlock::iterator DE = MI.getParent()->end();
+ DI != DE; ++DI) {
+ if (!DI->isDebugValue())
----------------
how about `for(MachineInstr &DMI : make_range(DI, DE)) { ... }`
================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:111
+}
+} // namespace
+
----------------
`// end anonymous namespace`
================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:114-115
+bool LiveRangeShrink::runOnMachineFunction(MachineFunction &MF) {
+ if (MF.empty())
+ return false;
+
----------------
This seems unnecessary the for loop below will abort shortly enough if the function is empty. Completely empty functions are also extremely rare so that this shortcut probably takes more time than it saves.
================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:124
+ continue;
+ InstOrderMap IOM;
+ bool SawStore = BuildInstOrderMap(MBB.instr_begin(), IOM);
----------------
I would have expected this outside the loop so it can be reused (considering that you also went through the trouble of `clear()`ing it inside BuildInstOrderMap.
================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:125
+ InstOrderMap IOM;
+ bool SawStore = BuildInstOrderMap(MBB.instr_begin(), IOM);
+
----------------
Collecting the SawStore value is unnecessary. The only user appears to be `MachineInstr::isSafeToMove()` below which writes(!) the variable.
================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:133-134
+
+ for (MachineBasicBlock::instr_iterator Next = MBB.instr_begin();
+ Next != MBB.instr_end();) {
+ MachineInstr *MI = &*Next;
----------------
Use `MachineBasicBlock::iterator`
================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:135
+ Next != MBB.instr_end();) {
+ MachineInstr *MI = &*Next;
+ ++Next;
----------------
Use a reference for things that cannot be `nullptr`.
================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:142
+ int Barrior = 0;
+ for (const auto &MO : MI->operands()) {
+ if (!MO.isReg() || MO.isDebug())
----------------
No need for `auto` here just write `MachineOperand`
================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:150
+ // be moved above Barrior.
+ Barrior = std::max(Barrior, UseMap[MO.getReg()]);
+ }
----------------
Barrier.
================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:156-157
+ // from moved before this MI.
+ if (MI->hasUnmodeledSideEffects() && Next != MBB.instr_end())
+ BuildInstOrderMap(Next, IOM);
+ if (!MI->isSafeToMove(nullptr, SawStore))
----------------
You could move this check into the `!isSafeToMove()` path.
================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:166
+ // live-ranges that are defined by a COPY as it could be coalesced later.
+ int NumEligibleUse = 0;
+
----------------
`unsigned` for counters.
================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:168
+
+ for (const auto &MO : MI->operands()) {
+ if (!MO.isReg() || MO.isDead() || MO.isDebug())
----------------
Avoid `auto`.
================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:173-174
+ // unless it is a constant physical register.
+ if (TargetRegisterInfo::isPhysicalRegister(MO.getReg()) &&
+ !MRI.isConstantPhysReg(MO.getReg())) {
+ Insert = nullptr;
----------------
Maybe assign MO.getReg() to a variable it is used 5 times here.
================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:211-215
+ MBB.splice(I, &MBB, MI->getIterator());
+ for (MachineInstr *DI : DbgValuesToMove) {
+ IOM[DI] = IOM[MI];
+ MBB.splice(I, &MBB, DI->getIterator());
+ }
----------------
There's a variant of splice that can move a sequence of instructions which should work nicely here.
It is also not clear to me if moving the debug instructions is legal here. They are not considered when checking how early an instruction can move so you are possibly moving the debug instruction above a point where the value its operand is defined.
https://reviews.llvm.org/D32563
More information about the llvm-commits
mailing list