[PATCH] Generate DWARF stack frame information for ARM/Thumb functions [2nd attempt].

[Saleem Abdulrasool]

================
Comment at: lib/CodeGen/AsmPrinter/ARMException.cpp:66
@@ +65,3 @@
+  AsmPrinter::CFIMoveType MoveType = Asm->needsCFIMoves();
+  assert (MoveType != AsmPrinter::CFI_M_EH);
+  if (MoveType == AsmPrinter::CFI_M_Debug) {
----------------
no space after the assert, and add a message please.

    assert(MoveType != AsmPrinter::CFI_M_EH &&
           "non-EH CFI move in prologue");

Although, Im not sure I fully understand this assertion.  Why is it impossible to encounter a debug CFI?  Or is it just that the lowering will always produce the ARM specific macros for the prologue setup?

================
Comment at: lib/CodeGen/AsmPrinter/ARMException.cpp:67
@@ +66,3 @@
+  assert (MoveType != AsmPrinter::CFI_M_EH);
+  if (MoveType == AsmPrinter::CFI_M_Debug) {
+    shouldEmitMoves = true;
----------------
Why not merge the next conditional execution statement into this condition?

================
Comment at: lib/Target/ARM/ARMFrameLowering.cpp:133
@@ -131,1 +132,3 @@
 
+static int sizeOfSPAdjustment(MachineInstr *MI) {
+  assert(MI->getOpcode() == ARM::VSTMDDB_UPD);
----------------
constify MI.

================
Comment at: lib/Target/ARM/ARMFrameLowering.cpp:138
@@ +137,3 @@
+  // pred) so the list starts at 4.
+  for (int i = MI->getNumOperands() - 1; i >= 4; --i) {
+    count += 8;
----------------
braces arent needed

================
Comment at: lib/Target/ARM/ARMFrameLowering.cpp:164
@@ -149,2 +163,3 @@
   unsigned FramePtr = RegInfo->getFrameRegister(MF);
+  int CfaOffset = 0;
 
----------------
I believe that CFAOffset would be more inline with the style.

================
Comment at: lib/Target/ARM/ARMFrameLowering.cpp:197
@@ +196,3 @@
+      CfaOffset -= NumBytes;
+      MMI.addFrameInst(
+          MCCFIInstruction::createDefCfaOffset(SPLabel, CfaOffset));
----------------
I think you can split on the , in the createDefCfaOffset instead.

================
Comment at: lib/CodeGen/AsmPrinter/ARMException.cpp:41
@@ -41,1 +40,3 @@
+  : DwarfException(A),
+    shouldEmitMoves(false) {}
 
----------------
This really controls CFI directives, I think that shouldEmitCFI would be more appropriate.

================
Comment at: lib/Target/ARM/ARMFrameLowering.cpp:364
@@ +363,3 @@
+        .addSym(SPLabel);
+    if(FramePtrOffsetInPush) {
+      CfaOffset += FramePtrOffsetInPush;
----------------
Missing space after if.

================
Comment at: lib/Target/ARM/ARMFrameLowering.cpp:150
@@ -137,1 +149,3 @@
+  MachineModuleInfo &MMI = MF.getMMI();
+  const MCRegisterInfo *MRI = MMI.getContext().getRegisterInfo();
   const ARMBaseRegisterInfo *RegInfo =
----------------
The MCContext seems to be used a bit as well.  Perhaps save off a reference to that as well?

================
Comment at: lib/Target/ARM/ARMFrameLowering.cpp:379
@@ +378,3 @@
+        .addSym(SPLabel);
+    if(!HasFP) {
+      CfaOffset -= GPRCS2Size;
----------------
Missing space after if.

================
Comment at: lib/Target/ARM/ARMFrameLowering.cpp:409
@@ +408,3 @@
+      MachineBasicBlock::iterator Push = DPRCSPush++;
+      if(!HasFP) {
+        SPLabel = MMI.getContext().CreateTempSymbol();
----------------
missing space after if

================
Comment at: lib/Target/ARM/ARMFrameLowering.cpp:419
@@ +418,3 @@
+
+    if(!SPLabel) {
+      SPLabel = MMI.getContext().CreateTempSymbol();
----------------
missing space after if

================
Comment at: lib/Target/ARM/ARMFrameLowering.cpp:436
@@ +435,3 @@
+      case ARM::R7:
+      case ARM::LR:
+      case ARM::R8:
----------------
Can you move LR after 12 so that the registers are in order?

================
Comment at: lib/Target/ARM/ARMFrameLowering.cpp:446
@@ +445,3 @@
+          MMI.addFrameInst(MCCFIInstruction::createOffset(SPLabel,
+              MRI->getDwarfRegNum(Reg, true), MFI->getObjectOffset(FI)));
+      }
----------------
This is not conformant to LLVM style.

================
Comment at: lib/Target/ARM/ARMFrameLowering.cpp:451
@@ +450,3 @@
+
+  if(NumBytes) {
+    if(!HasFP) {
----------------
missing space after if here and on the next line

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:137
@@ -118,1 +136,3 @@
 
+  if (ArgRegsSaveSize) {
+  }
----------------
Did you mean to leave this in?   I think the case has been fully handled above.

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:196
@@ -172,1 +195,3 @@
 
+  MCSymbol *SPLabel = MMI.getContext().CreateTempSymbol();
+  BuildMI(MBB, MBBI, dl, TII.get(TargetOpcode::PROLOG_LABEL)).addSym(SPLabel);
----------------
Its a shame to see the duplication for the label handling across the TUs.

================
Comment at: lib/Target/ARM/ARMFrameLowering.cpp:323
@@ +322,3 @@
+      int FI = I->getFrameIdx();
+      switch (Reg) {
+      case ARM::R0:
----------------
I would swap the order of the registers, check the high range first, if the STI is Darwin, break.  Otherwise fall through to the low registers, and emit the CFI.

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:207
@@ +206,3 @@
+    int FI = I->getFrameIdx();
+    switch (Reg) {
+    case ARM::R0:
----------------
Similar to the ARM handling, I would swap the check order and use fall-through behaviour to avoid the emission duplication.

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:244
@@ +243,3 @@
+        .addSym(SPLabel);
+    if(FramePtrOffsetInBlock) {
+      CfaOffset += FramePtrOffsetInBlock;
----------------
Missing space after if


http://llvm-reviews.chandlerc.com/D2706