[PATCH] D20769: [IPRA] Interprocedural Register Allocation

[Hal Finkel via llvm-commits]

hfinkel added inline comments.

================
Comment at: include/llvm/CodeGen/PhysicalRegisterUsageInfo.h:10-11
@@ +9,4 @@
+//===----------------------------------------------------------------------===//
+//
+// This pass is required to get Interprocedural Register Allocation effect.
+// See other two passes at lib/Target/X86/X86RegUsageInfoPropagate.cpp and
----------------
How about:
  // This pass is required to take advantage of the interprocedural-register-allocation infrastructure.

================
Comment at: include/llvm/CodeGen/PhysicalRegisterUsageInfo.h:15
@@ +14,3 @@
+//
+// This pass is simple immutable pass which keeps StringMap of
+// function name to RegMask (calculated based on actual register allocation) and
----------------
If you change from using a StringMap, update this comment.

================
Comment at: include/llvm/CodeGen/PhysicalRegisterUsageInfo.h:54
@@ +53,3 @@
+  // and 1 means content of register will be preserved around function call
+  StringMap<std::vector<uint32_t>> RegMasks;
+};
----------------
mehdi_amini wrote:
> MatzeB wrote:
> > @Mehdi: I've seen you arguing for a stringmap here before, but I would argue that a reference to a compiler internal object would be more stable here, avoid string comparisons and also work with unnamed functions (not sure if that actually allowed in llvm, but I could see this happening for JITs...).
> > 
> > As to what object to use: MachineFunction* would be nice but I am not sure that would well as long as the MachineFunction gets created in the MachineFunctionAnalysisPass. So maybe tie it to the Functions GlobalObject for now and change it to MachineFunction* later when we switch to the new pass manager and hopefully can deal with MachineFunctions as a first-class object rather than an analysis.
> I pointed a `StringMap` in order to not have the backend depends on the IR, now if you and Quentin thinks it is fine to go through a `GlobalVariable *`, I don't have strong feeling about it.
> (and yes, unnamed global function are allowed)
I'd also prefer a `GlobalVariable *` here, as that currently establishes our canonical function identify. If we work to further separate the MI level from the IR level, I suspect we'd establish some other function object which would have pointer-based identify, and we can always then update this code to use that instead.


================
Comment at: lib/CodeGen/PhysicalRegisterUsageInfo.cpp:11
@@ +10,3 @@
+//
+// This pass is required to get Interprocedural Register Allocation effect.
+// See other two passes at lib/Target/X86/X86RegUsageInfoPropagate.cpp and
----------------
MatzeB wrote:
> Use doxygen comments.
No need to duplicate this comment in both the source file and the header.

================
Comment at: lib/CodeGen/RegUsageInfoCollector.cpp:88
@@ +87,3 @@
+      (TRI->getNumRegs() + 31) / 32; // size should be in multiple of 32.
+  RegMask.resize(regMaskSize, 0);
+
----------------
MatzeB wrote:
> how about `uint32_t RegMask[regMaskSize]` instead of using a std::vector here so we get a stack allocation instead of an unnecessary heap allocation of the vector?
regMaskSize is not a constant, and we can't use VLAs. We could use SmallVector with a reasonable default, however.

================
Comment at: lib/CodeGen/TargetPassConfig.cpp:604
@@ -570,1 +603,3 @@
 
+  if (UseIPRA) {
+    // this is MachineFunction pass which iterates through register
----------------
Why here? It seems much too early. Backends can use the register scavenger to use otherwise-unused registers until the very end. I think this needs to be after the call below to:

  addPreEmitPass();

if not at the very end.

================
Comment at: lib/Target/X86/X86TargetMachine.cpp:283
@@ +282,3 @@
+  if (UseIPRA)
+    addPass(createRegUsageInfoPropPass());
+
----------------
Does this need to be here so that targets can opt-in? Could we reasonably schedule this early in TargetPassConfig::addMachinePasses?



http://reviews.llvm.org/D20769