[PATCH] D20769: [IPRA] Interprocedural Register Allocation

[Mehdi AMINI via llvm-commits]

mehdi_amini added a comment.

We're getting there, the patch is looking really good now! :)

Still a little bit of work ahead, see inline comments.


================
Comment at: include/llvm/Analysis/CallGraphSCCPass.h:115
@@ -113,2 +114,3 @@
 
-} // End llvm namespace
+class DummyCGSCCPass : public CallGraphSCCPass {
+public:
----------------
Describe the class with a doxygen  comment.

================
Comment at: include/llvm/CodeGen/MachineOperand.h:537
@@ -536,1 +536,3 @@
 
+  /// setRegMask - Sets value of register mask operand referencing Mask.  The
+  /// operand does not take ownership of the memory referenced by Mask, it must
----------------
Do not repeat the name of the function in the doxygen comment (other places that do it are outdated).

================
Comment at: include/llvm/CodeGen/Passes.h:366
@@ +365,3 @@
+  /// This pass is executed POST-RA to collect which physical registers are
+  /// used by given machine function.
+  FunctionPass *createRegUsageInfoCollector();
----------------
s/used/preserved/

================
Comment at: include/llvm/CodeGen/RegisterUsageInfo.h:13
@@ +12,3 @@
+///
+/// This pass is simple immutable pass which keeps DenseMap of
+/// GlobalVariable * for Function to RegMask (calculated based on actual 
----------------
You shouldn't introduce implementation details in high-level description.

================
Comment at: lib/CodeGen/CMakeLists.txt:103
@@ -102,2 +102,3 @@
   RegisterScavenging.cpp
   RenameIndependentSubregs.cpp
+  RegisterUsageInfo.cpp
----------------
It is a good practice to decoupled software component. Having separate patches helps to make sure we indeed have correctly separated the components.
It also forces to make sure the component are individually testable, and make sure we actually test them.

So I agree with Quentin on the principle, and I think it is also a good exercise for you to split the patch an submit the analysis alone and tested. Have a look at "CostModelAnalysis::print()" in  lib/Analysis/CostModel.cpp and see how it is tested in  test/Analysis/CostModel/X86/cast.ll



================
Comment at: lib/CodeGen/RegUsageInfoCollector.cpp:11
@@ +10,3 @@
+/// This pass is required to take advantage of
+/// the interprocedural-register-allocation infrastructure.
+///
----------------
Wrapping

================
Comment at: lib/CodeGen/TargetPassConfig.cpp:118
@@ +117,3 @@
+    UseIPRA("enable-ipra", cl::init(false), cl::Hidden,
+            cl::desc("Enable compile time interprocedural register allocation "
+                     "to reduce load/store at procedure calls."));
----------------
Why "compile time"? I'd just write "Enable inter-procedural register allocation"

================
Comment at: lib/CodeGen/TargetPassConfig.cpp:515
@@ -490,1 +514,3 @@
+//                       false)
+
 /// Add common passes that perform LLVM IR to IR transforms in preparation for
----------------
You forgot to remove this hunk


http://reviews.llvm.org/D20769