yroux marked 5 inline comments as done.
yroux added a comment.

Thanks for the review Sam,

I'll update the patch with more tests (for CPSR/LR it was part of D76068 
<https://reviews.llvm.org/D76068> but you are right it should be done here) and 
take your comments into account.

In D76066#1921331 <https://reviews.llvm.org/D76066#1921331>, @samparker wrote:

> 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);
----------------
samparker wrote:
> When you say 'available', do you mean 'dead'? I'm struggling to follow the 
> logic here...
Yes that's it,  'available' in llvm::LiveRegUnit means no part of the register 
is live so it's 'dead'.

The logic is to identify the unsafe cases where R12 or CPSR are live across the 
MBB without being defined in that MBB



================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:5663
+  // Now, add the live outs to the set.
+  LRU.addLiveOuts(MBB);
+
----------------
samparker wrote:
> Do you need to clear LRU first?
hmm, I don't think it is necessary to clear it, given that if the register was 
in the set the XXavailableInBlock is false so it doesn't matter if it is among 
liveouts or not the candidate will not have the flag UnsafeRegsDead and thus 
will not be outlined, if it wasn't and is part of the liveouts, we will reture 
false and the generic part of the MachineOutliner will not map the MBB and we 
will not outliune it.


================
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 ||
----------------
samparker wrote:
> I'm surprised to see IT here?
Yes, sorry I need to update the comment, this is a conservative way of handling 
IT blocks, to avoid having it splitted between an outlined region and the call 
sites


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


================
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))
----------------
samparker wrote:
> Looks like this MI building could be refactored.
yes, I'm refactoring it


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76066



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to