[PATCH] [mips] Expand JAL instructions when PIC is enabled.

[Daniel Sanders]

In http://reviews.llvm.org/D6231#190332, @tomatabacu wrote:

> In http://reviews.llvm.org/D6231#187264, @dsanders wrote:
>
> > Looking back through the comments, I believe the remaining issues are:
> >
> > - Duplicated nops
>
>
> I'm not sure if this is accurate. I've compared IAS and GAS with .set reorder/noreorder and
>  with/without manually added NOPs and I haven't seen any discrepancies.


In that case it must have been fixed recently.

> > - The handling of .text

> 

> 

> I have found a fix for this, but it's a bit ugly and it only touches common code.

>  If I include it here, it might slow down this patch even more.


I'm ok with leaving that for another patch.

> > - Offsets are lost for label+offset

> 

> 

> They used to be lost, but since I switched the code to use evaluateRelocExpr() offsets will cause a fatal error,

>  because it tries to inappropriately apply relocations to constants.

> 

> I'd rather have this than silently generating the wrong code.

>  However, evaluateRelocExpr() needs to be fixed.


I agree that an error is better than silently producing bad code and that evaluateRelocExpr() is badly flawed and needs fixing..

> Some other remaining problems are:

> 

> - no support for the LargeGOT case


That's ok for this patch, but following the principle above I'd suggest asserting that LargeGOT is not in use.

> - no support for forward-declared locals


Didn't we establish earlier in the review that these worked as expected? Has that changed?

> Also, this patch will cause a recently added RuntimeDyld test to fail (ExecutionEngine/RuntimeDyld/Mips/ELF_Mips64r2N64_PIC_relocations.s).

>  I don't know how to fix this test.


You need to disable pic for the 'jal foo' on line 44 using '.option pic0' and '.option pic2'. At the moment, it's trying to read the bottom 26-bits of a jal instruction but the new expansion means it's really reading the bottom 26-bits of an lw.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1273-1274
@@ +1272,4 @@
+static const MCSymbol *getSingleMCSymbol(const MCExpr *Expr) {
+  if (const MCSymbolRefExpr *SRExpr = dyn_cast<MCSymbolRefExpr>(Expr)) {
+    return &(SRExpr->getSymbol());
+  }
----------------
Nit: Unnecessary braces

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1461
@@ +1460,3 @@
+  // This expansion is not in a function called by expandInstruction() because
+  // the psudo-instruction doesn't have a distinct opcode.
+  if ((Inst.getOpcode() == Mips::JAL || Inst.getOpcode() == Mips::JAL_MM) &&
----------------
Nit: psudo->pseudo

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1468-1473
@@ +1467,8 @@
+
+    // We can do this expansion if there's only 1 symbol in the argument
+    // expression.
+    if (countMCSymbolRefExpr(JalExpr) > 1)
+      return Error(IDLoc, "jal doesn't support multiple symbols in PIC mode");
+
+    const MCSymbol *JalSym = getSingleMCSymbol(JalExpr);
+
----------------
I'm not sure I understand this bit. Can you explain?

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1511
@@ +1510,3 @@
+        MCInst LoadInst;
+        LoadInst.setOpcode(isABI_N64() ? Mips::LD : Mips::LW);
+        LoadInst.addOperand(MCOperand::createReg(Mips::T9));
----------------
Nit: isABI_N64() -> ABI.ArePtrs64bit()

http://reviews.llvm.org/D6231

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