[PATCH] D76066: [ARM][MachineOutliner] Add Machine Outliner support for ARM

[Sam Parker via Phabricator via cfe-commits]

samparker added a comment.

Hi,

I like reading your code! But I'm concerned around the lack of tests here, specifically:

- CPSR liveness.
- LR liveness.
- All things PIC related.
- Linkage legality.
- A negative test for Thumb-1.
- Inline asm generally concerns me too.



================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:5653
+
+  // Check if each of the unsafe registers are available...
+  bool R12AvailableInBlock = LRU.available(ARM::R12);
----------------
When you say 'available', do you mean 'dead'? I'm struggling to follow the logic here...


================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:5663
+  // Now, add the live outs to the set.
+  LRU.addLiveOuts(MBB);
+
----------------
Do you need to clear LRU first?


================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:5705
+  unsigned Opc = MI.getOpcode();
+  if (Opc == ARM::t2IT || Opc == ARM::tPICADD || Opc == ARM::PICADD ||
+      Opc == ARM::PICSTR || Opc == ARM::PICSTRB || Opc == ARM::PICSTRH ||
----------------
I'm surprised to see IT here?


================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:5761
+
+  // Don't touch the link register
+  if (MI.readsRegister(ARM::LR, &getRegisterInfo()) ||
----------------
Isn't this already handled in the loop above? I prefer this notation though.


================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:5781
+    if (Subtarget.isThumb())
+      if (Call->getOperand(2).isReg())
+        BuildMI(MBB, MBB.end(), DebugLoc(), get(ARM::tTAILJMPr))
----------------
Looks like this MI building could be refactored.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76066