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

[Daniel Sanders]

Sorry for taking so long on these. I had to find out how this is supposed to work before I could comment.

A couple general comments on the patch:

- There's no mention of -mxgot. I'm happy for -mxgot to be a follow up patch but could you insert assertions/unreachables to guard against it for now.
- Not required but consider refactoring into smaller functions. It's difficult to see the overall logic at the moment.
- You'll need to change the commands to use -target-abi


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1326-1329
@@ +1325,6 @@
+      inPicMode()) {
+    MCOperand JalOpnd = Inst.getOperand(0);
+    const MCSymbolRefExpr *JalSymExpr =
+        cast<MCSymbolRefExpr>(JalOpnd.getExpr());
+    const MCSymbol &JalSym = JalSymExpr->getSymbol();
+
----------------
Nit: This would be a little tidier as a single statement. We never use JalOpnd, and JalSymExpr again.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1331
@@ +1330,3 @@
+
+    if (JalSym.isInSection()) {
+      if (isABI_O32()) {
----------------
I doubt this condition is right, although it's probably close enough to cover a lot of cases.

Things that spring to mind:
* Are forward-declared locals handled correctly?
* Are weak symbols handled correctly?
* What if the symbol is a section name like .text?

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1343-1351
@@ +1342,11 @@
+        LwInst.addOperand(MCOperand::CreateReg(Mips::GP));
+        if (inMicroMipsMode()) {
+          const MCSymbolRefExpr *Got16RelocSymExpr = MCSymbolRefExpr::Create(
+              JalSym.getName(), MCSymbolRefExpr::VK_Mips_GOT, getContext());
+          LwInst.addOperand(MCOperand::CreateExpr(Got16RelocSymExpr));
+        } else {
+          const MCSymbolRefExpr *Got16RelocSymExpr = MCSymbolRefExpr::Create(
+              ".text", MCSymbolRefExpr::VK_Mips_GOT, getContext());
+          LwInst.addOperand(MCOperand::CreateExpr(Got16RelocSymExpr));
+        }
+        Instructions.push_back(LwInst);
----------------
These paths should be merged and we should use JalSym in both cases. The .text you see in objdump's output stems from a GAS optimisation to reduce the number of symbols it has to maintain.

Also, use JalSym instead of JalSym.getName() so we don't lose the offset on things like 'jal foo+4'

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1358-1364
@@ +1357,9 @@
+        AddiuInst.addOperand(MCOperand::CreateReg(Mips::T9));
+        if (inMicroMipsMode()) {
+          const MCSymbolRefExpr *Lo16RelocSymExpr = MCSymbolRefExpr::Create(
+              JalSym.getName(), MCSymbolRefExpr::VK_Mips_ABS_LO, getContext());
+          AddiuInst.addOperand(MCOperand::CreateExpr(Lo16RelocSymExpr));
+        } else {
+          const MCSymbolRefExpr *Lo16RelocSymExpr = MCSymbolRefExpr::Create(
+              ".text", MCSymbolRefExpr::VK_Mips_ABS_LO, getContext());
+          AddiuInst.addOperand(MCOperand::CreateExpr(Lo16RelocSymExpr));
----------------
Same as above

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1375
@@ +1374,3 @@
+        MCInst LwInst;
+        LwInst.setOpcode(Mips::LW);
+        LwInst.addOperand(MCOperand::CreateReg(Mips::T9));
----------------
LD for N64

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1378-1387
@@ +1377,12 @@
+        LwInst.addOperand(MCOperand::CreateReg(Mips::GP));
+        if (inMicroMipsMode()) {
+          const MCSymbolRefExpr *GotDispRelocSymExpr = MCSymbolRefExpr::Create(
+              JalSym.getName(), MCSymbolRefExpr::VK_Mips_GOT_DISP,
+              getContext());
+          LwInst.addOperand(MCOperand::CreateExpr(GotDispRelocSymExpr));
+        } else {
+          const MCSymbolRefExpr *GotDispRelocSymExpr = MCSymbolRefExpr::Create(
+              ".text", MCSymbolRefExpr::VK_Mips_GOT_DISP, getContext());
+          LwInst.addOperand(MCOperand::CreateExpr(GotDispRelocSymExpr));
+        }
+        Instructions.push_back(LwInst);
----------------
Same as above

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1390-1403
@@ +1389,16 @@
+      }
+    } else {
+      // If it's an external/weak symbol, we expand to:
+      //  lw    $25, 0($gp)
+      //    R_MIPS_CALL16  label  /  R_MICROMIPS_CALL16  label
+      //  jalr  $25
+      MCInst LwInst;
+      LwInst.setOpcode(Mips::LW);
+      LwInst.addOperand(MCOperand::CreateReg(Mips::T9));
+      LwInst.addOperand(MCOperand::CreateReg(Mips::GP));
+      const MCSymbolRefExpr *Call16RelocSymExpr = MCSymbolRefExpr::Create(
+          JalSym.getName(), MCSymbolRefExpr::VK_Mips_GOT_CALL, getContext());
+      LwInst.addOperand(MCOperand::CreateExpr(Call16RelocSymExpr));
+      Instructions.push_back(LwInst);
+    }
+
----------------
Is this doing the right thing for N64?

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1406-1409
@@ +1405,6 @@
+    MCInst JalrInst;
+    if (inMicroMipsMode())
+      JalrInst.setOpcode(Mips::JALR_MM);
+    else
+      JalrInst.setOpcode(Mips::JALR);
+    JalrInst.addOperand(MCOperand::CreateReg(Mips::RA));
----------------
Nit: JalrInst.setOpcode(inMicroMipsMode() ? Mips::JALR_MM : Mips::JALR);

http://reviews.llvm.org/D6231

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