[PATCH] D20769: [IPRA] Interprocedural Register Allocation
Matthias Braun via llvm-commits
llvm-commits at lists.llvm.org
Tue May 31 17:51:33 PDT 2016
MatzeB added a subscriber: MatzeB.
MatzeB added a comment.
The general approach seems fine at a first glance. I haven't reviewed this in-depth for correctness yet though, as I first have a bunch of nit-picks and coding style issues:
================
Comment at: include/llvm/CodeGen/MachineOperand.h:537
@@ -536,1 +536,3 @@
+ void setRegMask(const uint32_t *RegMaskPtr) {
+ assert(isRegMask() && "Wrong MachineOperand mutator");
----------------
qcolombet wrote:
> 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.
+1
================
Comment at: include/llvm/CodeGen/PhysicalRegisterUsageInfo.h:1
@@ +1,2 @@
+//=- PhysicalRegisterUsageInfo.cpp - Register Usage Informartion Storage -*- C++
+//-*-=//
----------------
qcolombet wrote:
> .cpp => .h
Does all of this need to be exposed in a public header anyway? Wouldn't it be enough to have the initialization stuff in Passes.h and keep the actual pass definitions private to the .cpp file in this case?
(if we decide no, then I'd rename this to something shorter like RegisterUsageInfo...)
================
Comment at: include/llvm/CodeGen/PhysicalRegisterUsageInfo.h:11-17
@@ +10,9 @@
+//
+// This pass is required to get Interprocedural Register Allocation effect.
+// See other two passes at lib/Target/X86/X86RegUsageInfoPropagate.cpp and
+// lib/CodeGen/RegUsageInfoCollector.cpp
+//
+// This pass is simple immutable pass which keeps StringMap of
+// function name to RegMask (calculated based on actual register allocation) and
+// provides simple API to query this information.
+//
----------------
Should use doxygen style comment here (see coding convention). It may also be wise not to mention specific filenames here as those could be moved or renamed in the future and people generally do not catch comment like this when doing so.
================
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: 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.
================
Comment at: include/llvm/CodeGen/PhysicalRegisterUsageInfo.h:59
@@ +58,1 @@
+#endif
\ No newline at end of file
----------------
Check your editor settings so the newline at the end of file is added.
================
Comment at: lib/CodeGen/PhysicalRegisterUsageInfo.cpp:1-2
@@ +1,3 @@
+//=- PhysicalRegisterUsageInfo.cpp - Register Usage Informartion Storage -*- C++
+//-*-=//
+//
----------------
strange linebreak. I think in .cpp we also don't need those magic markers for the emacs users (my understanding is that they are just used in the .h files so emacs knows they contain C++ code and not just C code).
================
Comment at: lib/CodeGen/PhysicalRegisterUsageInfo.cpp:11-17
@@ +10,9 @@
+//
+// This pass is required to get Interprocedural Register Allocation effect.
+// See other two passes at lib/Target/X86/X86RegUsageInfoPropagate.cpp and
+// lib/CodeGen/RegUsageInfoCollector.cpp
+//
+// This pass is simple immutable pass which keeps StringMap of
+// function name to RegMask (calculated based on actual register allocation) and
+// provides simple API to query this information.
+//
----------------
Use doxygen comments.
================
Comment at: lib/CodeGen/RegUsageInfoCollector.cpp:1-2
@@ +1,3 @@
+//=- RegUsageInfoCollector.cpp - Register Usage Informartion Collector -*- C++
+//-*-=//
+//
----------------
strange linebreak.
================
Comment at: lib/CodeGen/RegUsageInfoCollector.cpp:11-19
@@ +10,11 @@
+//
+// This pass is required to get Interprocedural Register Allocation effect.
+// See other two passes at lib/Target/X86/X86RegUsageInfoPropagate.cpp and
+// lib/CodeGen/PhysicalRegisterUsageInfo.cpp
+//
+// This pass is simple MachineFunction pass which collects register usage
+// details by iterating through
+// each physical registers and checking MRI::isPhysRegUsed() then creates a
+// RegMask based on this details.
+// The pass then stores this RegMask in PhysicalRegisterUsageInfo.cpp
+//
----------------
doxygen comments.
================
Comment at: lib/CodeGen/RegUsageInfoCollector.cpp:55
@@ +54,3 @@
+};
+}
+
----------------
add "end of anonymous namespace" comment (see coding conventions).
================
Comment at: lib/CodeGen/RegUsageInfoCollector.cpp:60
@@ +59,3 @@
+INITIALIZE_PASS_BEGIN(RegUsageInfoCollector, "RegUsageInfoCollector",
+ "Register Usage Information Collector Pass", false, false)
+INITIALIZE_PASS_DEPENDENCY(PhysicalRegisterUsageInfo)
----------------
It is clear that this is about a pass so no need for the " Pass" suffix in the explanation.
================
Comment at: lib/CodeGen/RegUsageInfoCollector.cpp:76
@@ +75,3 @@
+bool RegUsageInfoCollector::runOnMachineFunction(MachineFunction &MF) {
+ bool changed = false;
+ MachineRegisterInfo *MRI = &MF.getRegInfo();
----------------
This is always false anyway, just use `return false;` instead of a variable.
================
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);
----------------
qcolombet wrote:
> 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.
We could make the code here and in MachineOperand more robust by having a "typedef uint32_t RegMaskType" and then using `sizeof(RegMaskType) * CHAR_BIT` instead of hardcoding 32...
Though as that also hits existing code in MachineOperand a separate patch would be warranted.
================
Comment at: lib/CodeGen/RegUsageInfoCollector.cpp:88
@@ +87,3 @@
+ (TRI->getNumRegs() + 31) / 32; // size should be in multiple of 32.
+ RegMask.resize(regMaskSize, 0);
+
----------------
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?
================
Comment at: lib/CodeGen/RegUsageInfoCollector.cpp:92
@@ +91,3 @@
+
+ for (unsigned PReg = 1; PReg < TRI->getNumRegs(); ++PReg) {
+ if (!MRI->reg_nodbg_empty(PReg)) {
----------------
We tend to introduce a new variable (like `PRegE = TRI->getNumRegs()`) in loops like this and compare with it to avoid getNumRegs() getting called in every iteration of the loop (see coding conventions).
================
Comment at: lib/CodeGen/RegUsageInfoCollector.cpp:111
@@ +110,3 @@
+
+ const uint32_t * CallPreservedMask = TRI->getCallPreservedMask(MF, MF.getFunction()->getCallingConv());
+
----------------
No space after the `*`
================
Comment at: lib/CodeGen/TargetPassConfig.cpp:116
@@ -114,1 +115,3 @@
+// Option to enable compile time interprocedural register allocation.
+cl::opt<bool>
----------------
I feel like this comment is just stating the very obvious and can be left out.
================
Comment at: lib/CodeGen/TargetPassConfig.cpp:118
@@ +117,3 @@
+cl::opt<bool>
+ UseIPRA("enable-ipra", cl::init(false),
+ cl::desc("Enable compile time interprocedural register allocation "
----------------
I think we can should add cl::Hidden for now until this is proven and stable.
================
Comment at: lib/CodeGen/TargetPassConfig.cpp:502
@@ +501,3 @@
+namespace {
+class DummyCGSCCPass : public CallGraphSCCPass {
+public:
----------------
> Looks like we'll need a home for this pass, can't really leave it there. I'm not sure where to put it yet.
Just put the pass into CallGraphSCCPass.h as it is not specific to codegen (just happens to be used
there)?
================
Comment at: lib/Target/X86/X86RegUsageInfoPropagate.cpp:93
@@ +92,3 @@
+}
+
+bool RegUsageInfoPropagationPass::runOnMachineFunction(MachineFunction &MF) {
----------------
> for(auto &MBB : MF) {
> for(auto &MI : MBB) {
Please do not use `auto` in contexts where it is not obvious what type it represents. It is friendlier for the readers to use `MachineBasicBlock &MBB` and `MachineInstr &MI` here.
================
Comment at: lib/Target/X86/X86RegUsageInfoPropagate.cpp:101
@@ +100,3 @@
+
+ bool changed = false;
+
----------------
We start local variables with uppercase letters (coding convention).
================
Comment at: lib/Target/X86/X86RegUsageInfoPropagate.cpp:103-104
@@ +102,4 @@
+
+ for (auto &MBB : MF) {
+ for (auto &MI : MBB) {
+ // is it a call instr ?
----------------
same comment as above.
================
Comment at: lib/Target/X86/X86RegUsageInfoPropagate.cpp:114
@@ +113,3 @@
+ auto updateRegMask = [&](StringRef FuncName) {
+ const auto *RegMask = PRUI->getRegUsageInfo(FuncName);
+ if (RegMask) { // else skip optimization
----------------
This does not seem complicated enough to me to warrant a lambda. What about:
```
const char *FuncName = nullptr;
if (Operand.isGlobal()) {
Name = Operand.getGlobal()->getName();
} else if (Operand.isSymbol()) {
Name = Operand.getGlobal()->getName();
}
if (FuncName != nullptr && RegMask) {
setRegMask(MI, &(*RegMask)[0]);
changed = true;
}
```
================
Comment at: lib/Target/X86/X86RegUsageInfoPropagate.cpp:128-129
@@ +127,4 @@
+
+ DEBUG(
+ dbgs()
+ << "Call Instrunction After Register Usage Info Propagation : \n");
----------------
odd line break (clang-format may be confused because of the macro...)
================
Comment at: lib/Target/X86/X86RegUsageInfoPropagate.cpp:131-132
@@ +130,4 @@
+ << "Call Instrunction After Register Usage Info Propagation : \n");
+ DEBUG(MI.print(dbgs()));
+ DEBUG(dbgs() << "\n");
+ }
----------------
`DEBUG(dbgs() << MI << '\n');
http://reviews.llvm.org/D20769
More information about the llvm-commits
mailing list