[PATCH] D20769: [IPRA] Interprocedural Register Allocation

[Mehdi AMINI via llvm-commits]

mehdi_amini added inline comments.

================
Comment at: lib/Target/X86/X86RegUsageInfoPropagate.cpp:93
@@ +92,3 @@
+}
+
+bool RegUsageInfoPropagationPass::runOnMachineFunction(MachineFunction &MF) {
----------------
MatzeB wrote:
> > 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.
> 
I feel  that it is clearly obvious that `MI` is a `MachineInstr` when reading `for(auto &MI : MBB) {`, I'll be explicit if it was something else. But I won't pick this fight :)

================
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
----------------
MatzeB wrote:
> 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;
> }
> ```
I guess it is a matter of personal style, I'd try to avoid this construct when possible, but I don't really care here.


http://reviews.llvm.org/D20769