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

[Toma Tabacu]

Regarding -mxgot, AFAICT it is never checked for in the IAS. There is a command-line option used in llc, but I can't find a way to check for it from MipsAsmParser.
In the meantime, I've added a FIXME comment.

Also, note that offsets do not work (e.g. jal label+8). I can get the value of the offset, but I can't use it as an operand for the ADDIU, in the case of O32,
or store it in the relocation addendum, in the case of N32/N64. I think we would need to improve relocation emission in order to properly handle offsets.


================
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();
+
----------------
dsanders wrote:
> Nit: This would be a little tidier as a single statement. We never use JalOpnd, and JalSymExpr again.
Done.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1331
@@ +1330,3 @@
+
+    if (JalSym.isInSection()) {
+      if (isABI_O32()) {
----------------
dsanders wrote:
> 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?
Are forward-declared locals handled correctly?
Unfortunately, they are not (I assume you're talking about doing a "jal fwd_label" before the definition of fwd_label). They end up being treated as global symbols. I'm not sure how to fix this.

Are weak symbols handled correctly?
AFAICT weak symbols are treated the same way as global symbols.

What if the symbol is a section name like .text?
.text should be treated as a local symbol, but there is something weird happening: with llvm-objdump it's treated as a local symbol, but with plain llvm-mc, .text is treated as a global symbol.

================
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);
----------------
dsanders wrote:
> 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'
Done.

================
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));
----------------
dsanders wrote:
> Same as above
Done.

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

================
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);
----------------
dsanders wrote:
> dsanders wrote:
> > Same as above
> > Same as above
> 
> (I mean the above JalSym comment, not the N64 one)
Done.

================
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);
+    }
+
----------------
dsanders wrote:
> Is this doing the right thing for N64?
Except for not generating LD on N64 (now fixed), I believe so.

================
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));
----------------
dsanders wrote:
> Nit: JalrInst.setOpcode(inMicroMipsMode() ? Mips::JALR_MM : Mips::JALR);
Done.

http://reviews.llvm.org/D6231

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