[PATCH] D20769: [IPRA] Interprocedural Register Allocation

Quentin Colombet via llvm-commits llvm-commits at lists.llvm.org
Tue May 31 17:08:43 PDT 2016


qcolombet added a comment.

Hi Vivek,

A couple of minor comments, the main logic looks reasonable.

Cheers,
-Quentin


================
Comment at: include/llvm/CodeGen/MachineOperand.h:537
@@ -536,1 +536,3 @@
 
+  void setRegMask(const uint32_t *RegMaskPtr) {
+    assert(isRegMask() && "Wrong MachineOperand mutator");
----------------
I would say in a comment to double check MachineOperand::CreateRegMask for the characteristic of RegMaskPtr.

The reason why I think it is important is because that method clearly states the expected life time of the pointer and if we get it wrong we may end with nasty bug. Therefore, we should do our best to have the API clearly documented.

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

================
Comment at: include/llvm/CodeGen/PhysicalRegisterUsageInfo.h:1
@@ +1,2 @@
+//=- PhysicalRegisterUsageInfo.cpp - Register Usage Informartion Storage -*- C++
+//-*-=//
----------------
.cpp => .h

================
Comment at: include/llvm/CodeGen/PhysicalRegisterUsageInfo.h:2
@@ +1,3 @@
+//=- PhysicalRegisterUsageInfo.cpp - Register Usage Informartion Storage -*- C++
+//-*-=//
+//
----------------
Wrapped line.

================
Comment at: include/llvm/CodeGen/PhysicalRegisterUsageInfo.h:51
@@ +50,3 @@
+private:
+  // A simple map from function name to RegMask
+  // In RegMask 0 means register used (clobbered) by funciton
----------------
Period.

================
Comment at: include/llvm/CodeGen/PhysicalRegisterUsageInfo.h:51
@@ +50,3 @@
+private:
+  // A simple map from function name to RegMask
+  // In RegMask 0 means register used (clobbered) by funciton
----------------
qcolombet wrote:
> Period.
Please use doxygen style comment: ///

================
Comment at: include/llvm/CodeGen/PhysicalRegisterUsageInfo.h:52
@@ +51,3 @@
+  // A simple map from function name to RegMask
+  // In RegMask 0 means register used (clobbered) by funciton
+  // and 1 means content of register will be preserved around function call
----------------
Typo: function

================
Comment at: include/llvm/CodeGen/PhysicalRegisterUsageInfo.h:53
@@ +52,3 @@
+  // In RegMask 0 means register used (clobbered) by funciton
+  // and 1 means content of register will be preserved around function call
+  StringMap<std::vector<uint32_t>> RegMasks;
----------------
Period

================
Comment at: include/llvm/InitializePasses.h:341
@@ -340,2 +340,3 @@
 void initializePatchableFunctionPass(PassRegistry &);
+void initializePhysicalRegisterUsageInfoPass(PassRegistry &);
 }
----------------
I don’t know why this is not the case for last one, but we usually sort the initializers by name.

================
Comment at: lib/CodeGen/CMakeLists.txt:104
@@ -102,2 +103,3 @@
   RegisterScavenging.cpp
+  RegUsageInfoCollector.cpp
   SafeStack.cpp
----------------
Looks like this patch could be split:
- One patch for RegisterUsageInfo.
- One patch for InfoCollector.

================
Comment at: lib/CodeGen/PhysicalRegisterUsageInfo.cpp:27
@@ +26,3 @@
+
+#define DEBUG_TYPE "ipra"
+
----------------
ip-regalloc

================
Comment at: lib/CodeGen/PhysicalRegisterUsageInfo.cpp:45
@@ +44,3 @@
+    return &(RegMasks.find(MFName)->second);
+  else
+    return nullptr;
----------------
Remove the else per the coding convention.

================
Comment at: lib/CodeGen/RegUsageInfoCollector.cpp:16
@@ +15,3 @@
+// This pass is simple MachineFunction pass which collects register usage
+// details by iterating through
+// each physical registers and checking MRI::isPhysRegUsed() then creates a
----------------
Wrapping is strange here.
We split in the middle of a sentence without hitting the 80-col limit.

================
Comment at: lib/CodeGen/RegUsageInfoCollector.cpp:81
@@ +80,3 @@
+
+  DEBUG(dbgs() << " -------------------- Register Usage Collection Pass "
+                  "----------------------- \n");
----------------
Call getPassName.

================
Comment at: lib/CodeGen/RegUsageInfoCollector.cpp:87
@@ +86,3 @@
+  unsigned regMaskSize =
+      (TRI->getNumRegs() + 31) / 32; // size should be in multiple of 32.
+  RegMask.resize(regMaskSize, 0);
----------------
That is a bit strange to state the comment like that.
What about: Compute the size of the bit vector to represent all the registers. The bit vector is broken into 32-bit chunks, thus takes the ceil of the number of registers divided by 32 for the size.

================
Comment at: lib/CodeGen/RegUsageInfoCollector.cpp:94
@@ +93,3 @@
+    if (!MRI->reg_nodbg_empty(PReg)) {
+      if (MRI->isPhysRegUsed(PReg)) {
+        // If PReg is clobbered then all of its alias are also clobbered
----------------
&& instead of nested ifs.

================
Comment at: lib/CodeGen/RegUsageInfoCollector.cpp:95
@@ +94,3 @@
+      if (MRI->isPhysRegUsed(PReg)) {
+        // If PReg is clobbered then all of its alias are also clobbered
+        for (MCRegAliasIterator AI(PReg, TRI, true); AI.isValid(); ++AI) {
----------------
Period.

================
Comment at: lib/CodeGen/RegUsageInfoCollector.cpp:97
@@ +96,3 @@
+        for (MCRegAliasIterator AI(PReg, TRI, true); AI.isValid(); ++AI) {
+          RegMask[*AI / 32] |= 1u << *AI % 32;
+        }
----------------
Encapsulate that loop into a setRegister thing.

================
Comment at: lib/CodeGen/RegUsageInfoCollector.cpp:106
@@ +105,3 @@
+
+  // bitwise negation for RegMask because 0 means clobbred and 1 means preserved
+  for (unsigned index = 0; index < regMaskSize; index++) {
----------------
Capital letter at the beginning, period at the end of the sentence.

================
Comment at: lib/Target/X86/X86RegUsageInfoPropagate.cpp:2
@@ +1,3 @@
+//=- X86RegUsageInfoPropagate.cpp - Target Specific Register Usage Informartion
+//Propagation -*- C++ -*-=//
+//
----------------
Why is that pass X86 specific?
It should be generic, right?

================
Comment at: lib/Target/X86/X86RegUsageInfoPropagate.cpp:12
@@ +11,3 @@
+// effect.
+// See other two passes at lib/CodeGen/PhysicalRegisterUsageInfo.cpp and
+// lib/CodeGen/RegUsageInfoCollector.cpp
----------------
Use doxygen comments: ///

================
Comment at: lib/Target/X86/X86RegUsageInfoPropagate.cpp:16
@@ +15,3 @@
+// This pass iterates through MachineInstr in given MachineFunction and at each
+// callsite quires PhysicalRegisterUsageInfo.cpp
+// for RegMask (calculated based on actual register allocation) of callee
----------------
queries


http://reviews.llvm.org/D20769





More information about the llvm-commits mailing list