[PATCH] D16829: An implementation of Swing Modulo Scheduling
Matthias Braun via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 9 18:30:40 PDT 2016
MatzeB added a subscriber: MatzeB.
MatzeB added a comment.
There are probably several targets out there that care about software pipelining so it is good to code and share it between targets. Who will be the the code owner and conduct reviews/maintenance in the future?
I am not experienced with software pipelining algorithm so I mainly concentrated on coding style and llvm infrastructure issues. General comments:
- There is an odd mismatch between the class being 'MachineSMS' the filename being 'MachinePipeliner' the debug type 'modulo-sched' and the cl::opt switches being called 'swp-XXX'.
- While there are more than enough comments in the code, the pass could use some highlevel introduction/references to papers used etc.
Detailed comments:
================
Comment at: include/llvm/CodeGen/Passes.h:679-681
@@ -678,1 +678,5 @@
+
+ /// MachineSMS - This pass performs software pipelining on machine
+ /// instructions.
+ extern char &MachineSMSID;
} // End llvm namespace
----------------
Avoid repeating the name of the method/field in the doxygen comment as per coding convention.
(There's more instances of this following).
================
Comment at: include/llvm/Target/TargetInstrInfo.h:541-542
@@ +540,4 @@
+ /// used at the end.
+ virtual bool AnalyzeLoop(MachineLoop *L, MachineInstr *&IndVarInst,
+ MachineInstr *&CmpInst) const {
+ return true;
----------------
For parameters that cannot be nullptr I would recommend using a reference to make this fact clear (probably only MachineLoop *L here). Similar for the following functions.
================
Comment at: include/llvm/Target/TargetInstrInfo.h:1009-1010
@@ -985,1 +1008,4 @@
+ /// For instructions with a base and offset, return the position of the
+ /// base register and offset operands.
+ virtual bool getBaseAndOffsetPosition(const MachineInstr *MI,
----------------
The function returns just a bool but sets BasePos/OffsetPos.
================
Comment at: lib/CodeGen/MachinePipeliner.cpp:10-17
@@ +9,10 @@
+//
+// An implementation of the Swing Modulo Scheduling (SMS) software pipeliner.
+//
+// Software pipelining is an instruction scheduling technique for loops that
+// overlap loop iterations and explioits ILP via a compiler transformation.
+//
+// Swing Modulo Scheduling (SMS) is an implementation of software pipelining
+// that generates schedules that are near optimal in terms of initiation
+// interval, register requirements, and stage count.
+//
----------------
Use three slash doxygen.
================
Comment at: lib/CodeGen/MachinePipeliner.cpp:113-115
@@ +112,5 @@
+
+#ifndef NDEBUG
+ static int NumTries;
+#endif
+ // Cache the target analysis information about the loop.
----------------
Would be good to intregrate this into the OptBisect infrastructure in subsequent patches.
================
Comment at: lib/CodeGen/MachinePipeliner.cpp:131-132
@@ +130,4 @@
+ MachineSMS()
+ : MachineFunctionPass(ID), MF(nullptr), MLI(nullptr), MDT(nullptr),
+ TII(nullptr) {
+ initializeMachineSMSPass(*PassRegistry::getPassRegistry());
----------------
we can use C++11 member initializers now and write `MachineFunction *MF = nullptr;` above instead.
================
Comment at: lib/CodeGen/MachinePipeliner.cpp:170
@@ +169,3 @@
+ MachineSMS *Pass;
+ unsigned MII;
+ bool Scheduled;
----------------
This field could use a comment.
================
Comment at: lib/CodeGen/MachinePipeliner.cpp:191-193
@@ +190,5 @@
+
+ // A NodeSet contains a set of SUnit DAG nodes with additional information
+ // that assigns a priority to the set.
+public:
+ class NodeSet {
----------------
swap order of public: and the comment?
================
Comment at: lib/CodeGen/MachinePipeliner.cpp:194
@@ +193,3 @@
+public:
+ class NodeSet {
+ SetVector<SUnit *> Nodes;
----------------
Given that we are inside a .cpp file specific to modulo scheduling anyway we can probably move all those helper classes to the toplevel instead of nesting them into the class.
================
Comment at: lib/CodeGen/MachinePipeliner.cpp:211-214
@@ +210,6 @@
+
+ template <typename It>
+ NodeSet(It S, It E)
+ : Nodes(S, E), HasRecurrence(true), RecMII(0), MaxMOV(0), MaxDepth(0),
+ Colocate(0), ExceedPressure(nullptr) {}
+
----------------
Shouldn't you rather have the iterator implicitely cast to const_iterator so you do not need to have a templated version of the constructor (and insert etc. below)?
Could also use member initializers.
================
Comment at: lib/CodeGen/MachinePipeliner.cpp:303-315
@@ +302,15 @@
+private:
+ typedef SmallVector<NodeSet, 8> NodeSetType;
+ typedef DenseMap<unsigned, unsigned> ValueMapTy;
+ typedef SmallVectorImpl<MachineBasicBlock *> MBBVectorTy;
+ typedef DenseMap<MachineInstr *, MachineInstr *> InstrMapTy;
+
+ /// Instructions to change when emitting the final schedule.
+ DenseMap<SUnit *, std::pair<unsigned, int64_t>> InstrChanges;
+
+ /// We may create a new instruction, so remember it because it
+ /// must be deleted when the pass is finished.
+ SmallPtrSet<MachineInstr *, 4> NewMIs;
+
+ const RegisterClassInfo &RegClassInfo;
+
----------------
Why not move all those to the top of the class declaration to the other analysis info?
================
Comment at: lib/CodeGen/MachinePipeliner.cpp:359
@@ +358,3 @@
+
+ /// Clean up after the software pipeliner runs.
+ void finishBlock();
----------------
Many function comments are repeated here and at the place where the function is actually implemented. You should only mention the comment in one of the two places or risk that the two will get out of sync in time. (Which of the two you choose is a matter of personal preference and shouldn't matter).
================
Comment at: lib/CodeGen/MachinePipeliner.cpp:492
@@ +491,3 @@
+
+ // Group the remaining nodes into sets based upon connected components.
+ void groupRemainingNodes(NodeSetType &NodeSets);
----------------
Use doxygen comment (same for some following functions).
================
Comment at: lib/CodeGen/MachinePipeliner.cpp:811
@@ +810,3 @@
+bool MachineSMS::runOnMachineFunction(MachineFunction &mf) {
+
+ if (mf.getFunction()->getAttributes().hasAttribute(
----------------
I believe this pass is not required for correctness and should therefore have a `if(skipFunction(..)) return false;` sequence first.
================
Comment at: lib/CodeGen/MachinePipeliner.cpp:823-824
@@ +822,4 @@
+
+ for (MachineLoopInfo::iterator I = MLI->begin(), E = MLI->end(); I != E;
+ ++I) {
+ MachineLoop *L = *I;
----------------
Use range based for. Similar in many following loops.
================
Comment at: lib/CodeGen/MachinePipeliner.cpp:1702
@@ +1701,3 @@
+ dbgs() << "\t D = " << getDepth(&SUnits[i]) << "\n";
+ dbgs() << "\t H = " << getHeight(&SUnits[i]) << "\n";
+ }
----------------
It would be good to mention relevant papers in the comment at the beginning of the .cpp file.
================
Comment at: lib/CodeGen/MachinePipeliner.cpp:1815
@@ +1814,3 @@
+ SmallSet<unsigned, 4> Uses;
+ for (auto &I : NS) {
+ const MachineInstr *MI = I->getInstr();
----------------
Should avoid `auto` here and mention the type name. Some more instances following.
================
Comment at: lib/CodeGen/MachinePipeliner.cpp:3494
@@ +3493,3 @@
+ }
+ return 0;
+}
----------------
nullptr
================
Comment at: lib/CodeGen/Passes.cpp:113-116
@@ -112,2 +112,6 @@
+cl::opt<bool> EnableSWP("enable-swp", cl::Hidden, cl::init(false),
+ cl::ZeroOrMore,
+ cl::desc("Enable Software Pipelining"));
+
/// Allow standard passes to be disabled by command line options. This supports
----------------
Even though you see both, I think the more typical style nowadays would be to move this flag into the .cpp file of the pass and make it the first thing that is checked in runOnMachineFunction().
================
Comment at: lib/CodeGen/Passes.cpp:552-556
@@ -547,2 +551,7 @@
+ if (EnableSWP && getOptLevel() != CodeGenOpt::None) {
+ addPass(&MachineSMSID);
+ printAndVerify("After software pipelining");
+ }
+
// Run register allocation and passes that are tightly coupled with it,
----------------
Maybe we should leave adding the pass to the backends that support it and let them do it by overriding addPreRegAlloc()?
================
Comment at: test/CodeGen/Hexagon/swp-const-tc.ll:6
@@ +5,3 @@
+
+; CHECK: loop0(.LBB0_1, #998)
+
----------------
Some comments about all the tests:
- Tests should start with ; CHECK-LABEL: functionname: to make them more stable against unrelated output that happens to be on stdout triggering check lines (pass debug output, filenames, etc.)
- The tests appear to contain unnecessary extra data (nocapture, readonly) flags, sometimes strange value or block names, function and alias metadata. I am sure they can and should be further simplified.
================
Comment at: test/CodeGen/swp-multi-loops.ll:75-82
@@ +74,9 @@
+declare void @bar(i32*) #1
+
+attributes #0 = { nounwind "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf"="true" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf"="true" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #2 = { nounwind }
+
+!0 = !{!"int", !1}
+!1 = !{!"omnipotent char", !2}
+!2 = !{!"Simple C/C++ TBAA"}
----------------
Is all this metadata really necessary for the test?
http://reviews.llvm.org/D16829
More information about the llvm-commits
mailing list