[PATCH] D20769: [IPRA] Interprocedural Register Allocation

[Mehdi AMINI via llvm-commits]

mehdi_amini added inline comments.

================
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;
+};
----------------
hfinkel wrote:
> 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.
> 
Since MatzeB commented about the allocation in `std::vector` in the MachineFunctionPass, it seems equally important to override `doInitialization(Module &M)` in order to reserve the size of the map with the number of functions defined in the module, and `doFinalization` to clear the map.
(clearing the map is actually a correctness issue)

================
Comment at: lib/CodeGen/RegUsageInfoCollector.cpp:88
@@ +87,3 @@
+      (TRI->getNumRegs() + 31) / 32; // size should be in multiple of 32.
+  RegMask.resize(regMaskSize, 0);
+
----------------
hfinkel wrote:
> 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.
What is `regMaskSize` on ARM and X86?
Before moving on with stack allocation here, I think we have to consider that:

 - First this is ran once per function, so having one malloc per function during codegen does not make it an expensive analysis. 
 - Second the vector will be moved in the immutable pass map. So having a SmallVector makes it less inefficient to move and store (we may not want to have a DenseMap anymore, and thus we'd have to make an extra malloc there!).


http://reviews.llvm.org/D20769