[PATCH] D21180: [IPRA] Interprocedural Register Allocation - Transformation Pass

[Chad Rosier via llvm-commits]

mcrosier added inline comments.

================
Comment at: lib/CodeGen/RegUsageInfoPropagate.cpp:1
@@ +1,2 @@
+//=- RegUsageInfoPropagate.cpp - Register Usage Informartion Propagation -=//
+//
----------------
This doesn't appear to be 80-column.

================
Comment at: lib/CodeGen/RegUsageInfoPropagate.cpp:13
@@ +12,3 @@
+///
+/// This pass iterates through MachineInstr in given MachineFunction and at each
+/// callsite queries RegisterUsageInfo.cpp for RegMask (calculated based on
----------------
MachineInstr**s** in **a** given 

================
Comment at: lib/CodeGen/RegUsageInfoPropagate.cpp:14
@@ +13,3 @@
+/// This pass iterates through MachineInstr in given MachineFunction and at each
+/// callsite queries RegisterUsageInfo.cpp for RegMask (calculated based on
+/// actual register allocation) of callee function, if such details found then
----------------
RegisterUsageInfo.cpp -> RegisterUsageInfo

================
Comment at: lib/CodeGen/RegUsageInfoPropagate.cpp:15
@@ +14,3 @@
+/// callsite queries RegisterUsageInfo.cpp for RegMask (calculated based on
+/// actual register allocation) of callee function, if such details found then
+/// pass will update RegMask in call instruction. This updated RegMask will be
----------------
of **the** callee

Perhaps something like:
if the RegMask information is available then this pass will update the RegMask of the call instruction.

================
Comment at: lib/CodeGen/RegUsageInfoPropagate.cpp:17
@@ +16,3 @@
+/// pass will update RegMask in call instruction. This updated RegMask will be
+/// used by RegAllocators while allocating current MachineFunction.
+///
----------------
used by **the** register allocator while allocating **the** current MachineFunction.

================
Comment at: lib/CodeGen/RegUsageInfoPropagate.cpp:42
@@ +41,3 @@
+#define DEBUG_TYPE "ip-regalloc"
+
+namespace {
----------------
I've seen this style in the past:

#define RUIP_NAME "Register Usage Information Propagation"

and then replace the strings with the #define.

================
Comment at: lib/CodeGen/RegUsageInfoPropagate.cpp:53
@@ +52,3 @@
+  const char *getPassName() const override {
+    return "Register Usage Information Propagation";
+  }
----------------
  return  RUIP_NAME;

================
Comment at: lib/CodeGen/RegUsageInfoPropagate.cpp:75
@@ +74,3 @@
+                      "RegUsageInfoPropagationPass",
+                      "Register Usage Information Propagation", false, false)
+INITIALIZE_PASS_DEPENDENCY(PhysicalRegisterUsageInfo)
----------------
"Register Usage Information Propagation" -> RUIP_NAME

================
Comment at: lib/CodeGen/RegUsageInfoPropagate.cpp:78
@@ +77,3 @@
+INITIALIZE_PASS_END(RegUsageInfoPropagationPass, "RegUsageInfoPropagationPass",
+                    "Register Usage Information Propagation", false, false)
+
----------------
"Register Usage Information Propagation" -> RUIP_NAME

================
Comment at: lib/CodeGen/RegUsageInfoPropagate.cpp:102
@@ +101,3 @@
+    for (MachineInstr &MI : MBB) {
+      // is it a call instr ?
+      if (MI.isCall()) {
----------------
I'm not sure this comment is necessary, but I don't have any strong feelings for or against it.  However, if you do decide to keep it, please write all comments using proper English prose (capitalization, puncuation, etc.).

http://llvm.org/docs/CodingStandards.html#commenting

================
Comment at: lib/CodeGen/RegUsageInfoPropagate.cpp:103
@@ +102,3 @@
+      // is it a call instr ?
+      if (MI.isCall()) {
+        DEBUG(
----------------
You can reduce the indent by changing this check to:

  if (!MI.isCall())
    continue;

I tend to think it also improves readability.


http://reviews.llvm.org/D21180