[PATCH] [mips] Add assembler support for the .cprestore directive.

[Toma Tabacu]

Replied to inline comments.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:361
@@ -354,1 +360,3 @@
+
+    IsCpRestoreSet = false;
   }
----------------
dsanders wrote:
> Just to be on the safe side, could you also initialize CpRestoreOffset in the constructor?
Done.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1454
@@ +1453,3 @@
+
+    if (ExpandedJalSym == false)
+      return false;
----------------
dsanders wrote:
> Nit: if(!ExpandedJalSym)
Actually found a bug with this, so the "if" you commented about no longer exists, but I addressed the nit for the new code.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1654-1688
@@ -1638,2 +1653,37 @@
+
+  if ((Inst.getOpcode() == Mips::JalOneReg ||
+       Inst.getOpcode() == Mips::JalTwoReg || ExpandedJalSym) &&
+      inPicMode() && !(isABI_N32() || isABI_N64())) {
+    if (IsCpRestoreSet) {
+      // Emit a NOP after the JALR, even if .set reorder has not been used.
+      if (!AssemblerOptions.back()->isReorder()) {
+        MCInst NopInst;
+        if (hasShortDelaySlot(Inst.getOpcode()) ||
+            ((Inst.getOpcode() == Mips::JalOneReg ||
+              Inst.getOpcode() == Mips::JalTwoReg) &&
+             inMicroMipsMode())) {
+          NopInst.setOpcode(Mips::MOVE16_MM);
+          NopInst.addOperand(MCOperand::CreateReg(Mips::ZERO));
+          NopInst.addOperand(MCOperand::CreateReg(Mips::ZERO));
+        } else {
+          NopInst.setOpcode(Mips::SLL);
+          NopInst.addOperand(MCOperand::CreateReg(Mips::ZERO));
+          NopInst.addOperand(MCOperand::CreateReg(Mips::ZERO));
+          NopInst.addOperand(MCOperand::CreateImm(0));
+        }
+        Instructions.push_back(NopInst);
+      }
+
+      // Load the $gp from the stack.
+      SmallVector<MCInst, 3> LoadInsts;
+      createCpRestoreMemOp(true /*IsLoad*/, CpRestoreOffset /*StackOffset*/,
+                           IDLoc, LoadInsts);
+
+      for (const MCInst &Inst : LoadInsts)
+        Instructions.push_back(Inst);
 
+    } else {
+      Warning(IDLoc, "no .cprestore used in PIC mode");
+    }
+  }
   return false;
----------------
dsanders wrote:
> I'm a bit confused by this code. Emitting a nop after a jal/jalr/jalrs makes sense to me, but I don't understand why the presence of the nop depends on inPicMode() and (!N32 && !N64).
> 
> I'm also not sure why the non-macro versions of jalr/jalrs (which set ExpandedJalSym to true) are causing createCpRestoreMemOp() to be called.
.cprestore causes an LW of $gp to be emitted after an expanded "JAL symbol" (which becomes a JALR) and it does so only if we're in PIC mode and using the O32 ABI. Between that JALR and the LW, we emit a NOP, which will have the same dependencies as .cprestore.

In response to your 2nd comment, AFAICT, ExpandedJalSym is true only if we did the PIC expansion for a "JAL symbol".

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1658
@@ +1657,3 @@
+    if (IsCpRestoreSet) {
+      // Emit a NOP after the JALR, even if .set reorder has not been used.
+      if (!AssemblerOptions.back()->isReorder()) {
----------------
dsanders wrote:
> This comment isn't quite right. It's emitting a nop if we are in '.set noreorder' mode.
Fixed.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1806-1816
@@ -1750,9 +1805,13 @@
   if (AssemblerOptions.back()->isReorder()) {
-    // This is a 32-bit NOP because these 2 pseudo-instructions
-    // do not have a short delay slot.
     MCInst NopInst;
-    NopInst.setOpcode(Mips::SLL);
-    NopInst.addOperand(MCOperand::CreateReg(Mips::ZERO));
-    NopInst.addOperand(MCOperand::CreateReg(Mips::ZERO));
-    NopInst.addOperand(MCOperand::CreateImm(0));
+    if (hasShortDelaySlot(JalrInst.getOpcode())) {
+      NopInst.setOpcode(Mips::MOVE16_MM);
+      NopInst.addOperand(MCOperand::CreateReg(Mips::ZERO));
+      NopInst.addOperand(MCOperand::CreateReg(Mips::ZERO));
+    } else {
+      NopInst.setOpcode(Mips::SLL);
+      NopInst.addOperand(MCOperand::CreateReg(Mips::ZERO));
+      NopInst.addOperand(MCOperand::CreateReg(Mips::ZERO));
+      NopInst.addOperand(MCOperand::CreateImm(0));
+    }
     Instructions.push_back(NopInst);
----------------
tomatabacu wrote:
> dsanders wrote:
> > We ought to factor out the nop emission at some point
> Done in http://reviews.llvm.org/D8320. I'd rather wait until that gets committed before I update this one, instead of juggling around with all these patches.
Fully done.

http://reviews.llvm.org/D6267

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/