[PATCH] D20769: [IPRA] Interprocedural Register Allocation

[Mehdi AMINI via llvm-commits]

mehdi_amini added a comment.

Somehow I had unsubmitted comments on phab, some are still relevant.


================
Comment at: lib/CodeGen/PhysicalRegisterUsageInfo.cpp:2
@@ +1,3 @@
+//=- PhysicalRegisterUsageInfo.cpp - Register Usage Informartion Storage -*- C++
+//-*-=//
+//
----------------
This should be on one line (and fits within 80 chars).

================
Comment at: lib/CodeGen/RegUsageInfoCollector.cpp:2
@@ +1,3 @@
+//=- RegUsageInfoCollector.cpp - Register Usage Informartion Collector -*- C++
+//-*-=//
+//
----------------
same

================
Comment at: lib/CodeGen/RegUsageInfoCollector.cpp:37
@@ +36,3 @@
+namespace llvm {
+void initializeRegUsageInfoCollectorPass(PassRegistry &);
+}
----------------
Doesn't this need to be in a header and be called somewhere?
It's not clear to me how is this pass registered?

================
Comment at: lib/CodeGen/RegUsageInfoCollector.cpp:109
@@ +108,3 @@
+    RegMask[index] = ~RegMask[index];
+  }
+
----------------
Why not initializing RegMask with `0xFFFFFFFF` in `resize()` and having the loop setting the bit to zero instead?

================
Comment at: lib/CodeGen/RegUsageInfoCollector.cpp:115
@@ +114,3 @@
+    RegMask[index] = RegMask[index] | CallPreservedMask[index];
+  }
+
----------------
comment

================
Comment at: lib/CodeGen/TargetPassConfig.cpp:523
@@ +522,3 @@
+  if (UseIPRA) {
+    // FORCE CODEGEN TO RUN ACCORDING TO THE CALLGRAPH
+    initializeDummyCGSCCPassPass(*PassRegistry::getPassRegistry());
----------------
No capital letters here.

================
Comment at: lib/CodeGen/TargetPassConfig.cpp:605
@@ +604,3 @@
+  if (UseIPRA) {
+    // this is MachineFunction pass which iterates through register
+    // to check if it is used or not and creates RegMask
----------------
s/register/registers/

================
Comment at: lib/CodeGen/TargetPassConfig.cpp:606
@@ +605,3 @@
+    // this is MachineFunction pass which iterates through register
+    // to check if it is used or not and creates RegMask
+    addPass(createRegUsageInfoCollector());
----------------
Comment could be simply: `Collect register usage information and produce a register mask of clobbered registers, to be used to optimize call sites`.

================
Comment at: lib/Target/X86/X86.h:86
@@ -85,1 +85,3 @@
 
+/// Return a MachineFunction pass that identifies call site
+/// and propagates register usage information of callee to caller
----------------
s/call site/call sites/

================
Comment at: lib/Target/X86/X86RegUsageInfoPropagate.cpp:2
@@ +1,3 @@
+//=- X86RegUsageInfoPropagate.cpp - Target Specific Register Usage Informartion
+//Propagation -*- C++ -*-=//
+//
----------------
qcolombet wrote:
> Why is that pass X86 specific?
> It should be generic, right?
When we first looked at it, is seemed to me that we would have to switch over the various possible calls. But it was a mistake on my side, and now that it is implemented it does not seem to need any target specific bit indeed.


http://reviews.llvm.org/D20769