[PATCH] D79785: [ARM] Register pressure with -mthumb forces register reload before each call

[John Brawn via Phabricator via llvm-commits]

john.brawn added a comment.

Please add some tests for this to llvm/test/CodeGen/ARM/.



================
Comment at: llvm/lib/Target/ARM/Thumb1InstrInfo.cpp:173-174
+    assert(!CPE.isMachineConstantPoolEntry() && "Invalid constpool entry");
+    const Constant *callee = cast<Constant>(CPE.Val.ConstVal);
+    const char *func_name = MF.createExternalSymbolName(callee->getName());
+    MachineInstrBuilder MIB = BuildMI(*MI.getParent(), InsertPt, MI.getDebugLoc(), get(ARM::tBL))
----------------
Please use the standard variable naming convention here (i.e. Callee and FuncName).


================
Comment at: llvm/lib/Target/ARM/Thumb1InstrInfo.cpp:178-180
+                                      .addReg(ARM::R0, RegState::Implicit | RegState::Kill)
+                                      .addReg(ARM::R1, RegState::Implicit | RegState::Kill)
+                                      .addReg(ARM::R2, RegState::Implicit | RegState::Kill);
----------------
You can't assume that these are the registers that this function uses. You should be copying the implicit register operands of the original BLX. You also need to add the RegMask operand of the BLX, as otherwise the call is treated as preserving all registers.


================
Comment at: llvm/lib/Target/ARM/Thumb1InstrInfo.h:58
+  // foldMemoryOperand
+  using TargetInstrInfo::foldMemoryOperandImpl;
+
----------------
This doesn't do anything - this using declaration introduces TargetInstrInfo::foldMemoryOperandImpl as a public member of Thumb1InstrInfo, but you can override it as a public function (as is done here) without doing that. I doing see any reason to declare foldMemoryOperandImpl as public though, it should probably be declared as protected (like it is in TargetInstrInfo).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79785/new/

https://reviews.llvm.org/D79785