[PATCH] D30914: [Outliner] Add outliner for AArch64

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

This looks good! I only see one real problem right now, the rest is the usual coding style/nitpicking.

- ARM instructions only use a limited number of bits to encode immediate values. I don't see the stack fixup code checking whether the adjusted value still fits the instruction encodings.



================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:4240
+
+  auto &StackOffsetOperand = MI.getOperand(MI.getNumExplicitOperands() - 1);
+
----------------
Typing `MachineOperand &` here isn't that much longer and friendlier to the reader.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:4249-4323
+  case AArch64::LDURBBi:
+  case AArch64::LDURBi:
+  case AArch64::LDURDi:
+  case AArch64::LDURHHi:
+  case AArch64::LDURHi:
+  case AArch64::LDURQi:
+  case AArch64::LDURSBWi:
----------------
This is an awful lot of instruction specifics here. This list is dangerous to become stale/out of date when people add instruction in the future. Better try to move this into AArch64InstrInfo where the chance is higher that people will see and update it. Or better try to share code with one of the existing functions there (`AArch64InstrInfo::getMemOpBaseRegImmOfsWidth` looks similar, do you need modifications to the API to reuse it?)


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:4337-4339
+  // Don't outline LOHs.
+  if (FuncInfo->getLOHRelated().count(&MI))
+    return MachineOutlinerInstrType::Illegal;
----------------
That's a bit of a pity. Shouldn't be too hard to determine the effected LOH entries and throw them away as part of an upcoming patch (I would consider outlining more important than having a linker optimization hint).


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:4348
+
+    // Does its parent have any successors in its MachineFunction?
+    if (MI.getParent()->succ_empty())
----------------
Maybe make a slightly higher level statement: `// Does the function end here?`


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:4369-4370
+      MI.readsRegister(AArch64::LR, &RI) ||
+      MI.getDesc().hasImplicitUseOfPhysReg(AArch64::LR) ||
+      MI.getDesc().hasImplicitDefOfPhysReg(AArch64::LR))
+      return MachineOutlinerInstrType::Illegal;
----------------
Could you check whether you really need the getDesc() checks? I was hoping only the X86 target has these missing operand bugs... Same with the next if.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:4397
+      // Get the operand that uses the stack.
+      auto &StackOffsetOperand =
+      MI.getOperand(MI.getNumExplicitOperands() - 1);
----------------
Less `auto` please.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:4398
+      auto &StackOffsetOperand =
+      MI.getOperand(MI.getNumExplicitOperands() - 1);
+
----------------
The `getNumExplicitOperands()-1` seems somewhat magical, is there an AArch64InstrInfo function for it? If not consider creating one.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:4448-4471
+  // We're not tail calling, so we have to save LR before the call and restore
+  // it after.
+  MachineInstr *STRXpre = BuildMI(MF, DebugLoc(), get(AArch64::STRXpre))
+                              .addReg(AArch64::SP, RegState::Define)
+                              .addReg(AArch64::LR)
+                              .addReg(AArch64::SP)
+                              .addImm(-16);
----------------
Suggestion for followup patches: Use the LivePhysReg utility to discover unused registers in the outlined sequence so you can get away without a load/store in some cases.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.h:250
+                               bool CanBeTailCall) const override;
+  llvm::AArch64GenInstrInfo::MachineOutlinerInstrType
+  getOutliningType(MachineInstr &MI) const override;
----------------
The `llvm::` prefix is probably unnecessary.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.h:260
+  insertOutlinedCall(Module &M, MachineBasicBlock &MBB,
+                     MachineBasicBlock::iterator &It,
+                     MachineFunction &MF,
----------------
I should have noticed in the main outliner review already: When you use an instruction iterator to signal the insertion point, I'd recommend calling it `InsertBefore` or `InsertAfter` to make it obvious whether we insert before or after the given instruction...
(Feel free to fix this on the side without review)


https://reviews.llvm.org/D30914