[PATCH] D20769: [IPRA] Interprocedural Register Allocation

Matthias Braun via llvm-commits llvm-commits at lists.llvm.org
Tue May 31 18:34:45 PDT 2016


> On May 31, 2016, at 6:31 PM, Mehdi AMINI via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
> 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!).
Good point, optimizing heap vs. stack allocations in something computed only once per function is not worth it.

- Matthias



More information about the llvm-commits mailing list