[PATCH] D30971: [MIR] Support Customed Register Mask and CSRs

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

Thanks for working on this!

- Printing the regmask as hexnumbers seems like a bad idea: Adding or removing registers in between would render old .mir files invalid. Even though it's unwieldy I think we should print lists of register names instead. Printing the list of preserved registers instead of the clobbered ones may also make the lists shorter.

- I only very recently learned about the thing that was called "CalleeSavedRegisters" (or UnusedPhysRegs after your patch). It is silly that we serialize this information in the first place as you can trivially recompute it after loading a mir file: Loop over all blocks, instructions, operands and call MachineRegisterInfo::addPhysRegsUsedFromRegMask on every regmask operand you find. That way we have correct information and don't have to deal with printing and parsing this information in the first place. Maybe you could change this as part of this patch?

- Browsing around I found "@todo Need to add support in MIPrinter and MIParser to represent..." in the middle of X86 code which you can remove with this.



================
Comment at: include/llvm/CodeGen/MachineRegisterInfo.h:78-82
+  bool IsUpdatedCSRsInitialized;
 
   /// Contains the updated callee saved register list.
   /// As opposed to the static list defined in register info,
+  /// all registers that were disabled are removed from the list.
----------------
Unrelated and so trivial that you can just commmit this independently without review.


================
Comment at: lib/CodeGen/MIRParser/MIParser.cpp:1676
+  assert(Token.stringValue() == "CustomRegMask" && "Expected a custom RegMask");
+  const auto *TRI = MF.getSubtarget().getRegisterInfo();
+  assert(TRI && "Expected target register info");
----------------
Writing the type instead of `auto` is friendlier to readers (unless the type is immediately obvious, which it is not here).


================
Comment at: lib/CodeGen/MIRParser/MIParser.cpp:1682
+
+  // Expecting a register mask as a list of hexdecimal values.
+  APInt Result;
----------------
typo


================
Comment at: lib/CodeGen/MIRParser/MIParser.cpp:1683
+  // Expecting a register mask as a list of hexdecimal values.
+  APInt Result;
+  uint32_t *Mask = MF.allocateRegisterMask(TRI->getNumRegs());
----------------
Move the declaration to where it is used to avoid the impression that it needs to hold values accross loop iterations.


================
Comment at: lib/CodeGen/MIRPrinter.cpp:889-892
+    else if (Op.getRegMask())
+      printCustomRegMask(Op.getRegMask(), OS, TRI);
     else
       llvm_unreachable("Can't print this machine register mask yet.");
----------------
RegMask operands with a nullptr in the masks are illegal and it is perfectly reasonable not to test for that here and just crash in that case.


================
Comment at: lib/CodeGen/MachineRegisterInfo.cpp:593
+void MachineRegisterInfo::setCalleeSavedRegs(
+    const SmallVectorImpl<MCPhysReg> &CSRs) {
+  if (IsUpdatedCSRsInitialized)
----------------
Use `ArrayRef<MCPhysReg>` here for increased flexibility (smallvectors are among the things that automatically convert to ArrayRefs).


================
Comment at: lib/CodeGen/MachineRegisterInfo.cpp:597
+
+  for (auto Reg : CSRs)
+    UpdatedCSRs.push_back(Reg);
----------------
Please use MCPhysReg instead of auto.


Repository:
  rL LLVM

https://reviews.llvm.org/D30971