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

[Daniel Sanders]

================
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;
----------------
tomatabacu wrote:
> 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".
> .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.

Ah ok. So it's true when .cprestore has a visible effect on the output and false otherwise. That makes sense to me. In that case I'd suggest using inline functions to give this condition a more meaningful name (maybe shouldCprestoreModifyJal() or shouldRestoreGPAfterJal()?).

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

I see where my confusion is. jal is a macro that potentially expands to the jal instruction (or a sequence involving the jal instruction) and we're using the tablegen-erated assembly parsing for the jal instruction to parse the jal macro. The parsing gives us an MCInst which is the jal instruction but we expand it as if it were the jal macro to produce a jal instruction (or a sequence involving the jal instruction). Simple as that :-). It's a consequence of having the assembler and codegen in the same source which makes it difficult to separate the overlapping jal syntax. As confusing as it is, I can't think of a better way to go about it at the moment. I think we should comment on how this expansion works and how the line between instructions and macros are a bit blurry somewhere (the start of this if-statement?) and try to resolve the confusion later. Maybe, we can do something involving isCodeGenOnly and a assembler-only pseudo.

By the way, I've just noticed that we don't seem to be honouring '.set nomacro' properly. At least, I don't seem to be able to find any suitable guards near/in needsExpansion() and expandInstruction(). If we aren't honoring it then -fintegrated-as is likely to emit different objects for assembling via temporary assembly files and directly from codegen. Our codegen uses '.set nomacro' so we should fix this soon.

http://reviews.llvm.org/D6267

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