Try to handle 'dla' in PIC mode for N64.

[bsdjhb]

Summary

[MIPS] Handle PIC load address macro instructions in N64.

In particular, use CALL16 (similar to O32) for address loads into T9 for certain
cases. Otherwise use a %got_disp relocation to load the address of a symbol.
Small offsets (small enough to fit in a 16-bit signed immediate) can be used and
are added to the symbol address after it is loaded from the GOT. Larger offsets
are currently unsupported and result in an error from the assembler.

[bsdjhb]

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

2955 ↗

(On Diff #101590)

This comment is now stale and needs updating, I'm just not quite sure what the correct new comment is.

[sdardis]

I do not yet have test cases written (I'm not familiar with writing those for llvm),

If you look in test/MC/Mips/macro-la-pic.s , you'll find the test for the O32 'la' macro.

Copy that file to test/MC/Mips/macro-dla-pic.s, and modify the RUN: lines that mention -mtriple=mips-unknown-linux to -mtriple=mips64-unknown-linux and modify the -mcpu=mips32rX to -mcpu=mips64r6.

You'll need change the CHECK: lines to account for the change in instructions, as the encoding will be different due to using 'ld' instead of 'lw'. See http://llvm.org/docs/CommandGuide/FileCheck.html for more details on how FileCheck works.

You can run a single test by invoking llvm-lit from the build directory. e.g:

./bin/llvm-lit test/MC/Mips/macro-la-pic.s -a

Will run just that test and be verbose (the '-a') about it. You'll also see the commands use to run the test and use that to find the encodings for the different instructions.

I'll take a longer look at this tomorrow.

Thanks for doing this,
Simon

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

2928 ↗

(On Diff #101590)

The '&& isGP64bit()' is unnecessary, ArePtrs64bit() should be sufficient to cover N64.

2955 ↗

(On Diff #101590)

I'm not wholly sure right now myself, but if you grab a recent copy of binutils and experiment (you'll want to see what GCC produces for PIC code as a reference), you'll get something.

2966–2968 ↗

(On Diff #101590)

XXX -> FIXME. LLVM uses "FIXME"s over XXX or ???.

[sdardis]

Can you split out the change of choosing CALL16 vs GOT for O32 at least? It's not wholly relevant to this patch and IIRC correctly, there may be deficiency in the assembly parser regarding forward declared local symbols. I have a patch floating around that addresses that.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

2849 ↗

(On Diff #101590)

Can you add "FIXME: These expansions do not respect -mxgot."

2867–2872 ↗

(On Diff #101590)

This change should be submitted seperately.

2966–2968 ↗

(On Diff #101590)

N64 and N32 both use GOT_DISP, so the bottom FIXME can be removed.

2971 ↗

(On Diff #101590)

While the O32 ABI produces a R_MIPS_LO16 for symbols that are temporary or located in the current section, N64/ N32 produce a R_MIPS_GOT16 only.

2972–2976 ↗

(On Diff #101590)

This hunk and the addition when LoExpr != nullptr is incorrect when the constant is < -0x8000 || >0x7fff as the symbol offset is truncated. Furthermore, GAS indicates that the offset for a symbol is 32 bits are most.

[bsdjhb]

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

2972–2976 ↗

(On Diff #101590)

To be clear, does this mean that LoExpr should just be dropped entirely here? In the specific use cases I've seen this with FreeBSD there isn't an offset so I've not exercised this case (and am not sure what the right thing to do is)

[bsdjhb]

• Rebase on top of D33999
• A few fixes suggested by Simon Dardis.
• Add initial tests.
• Start on updating the comments, though I don't think this is quite right still.

[bsdjhb]

• Don't include the symbol offset in the %got_disp relocation.
• Add missing daddui's for symbol offsets to the test.
• Update comments and punt on large offsets for now.

[sdardis]

Some small changes requested, but otherwise this looks good.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

2972–2976 ↗

(On Diff #101590)

The handling of of a constant and a symbol here is correct for the case of a sign extended 16 bit number.

2868 ↗

(On Diff #103541)

Can you remove that change?

The expansion for 'la' and 'dla' in the PIC case are very similar. I'd like both chunks of code to be as similar as possible so that the refactoring is as easy as possible.

2943–2948 ↗

(On Diff #103541)

Formatting. See my comment on D33999.

2963–2964 ↗

(On Diff #103541)

Formatting. This should be:

const MipsMCExpr *GotExpr = MipsMCExpr::create(MipsMCExpr::MEK_GOT_DISP,
                                               Res.getSymA(), getContext());

2972 ↗

(On Diff #103541)

FIXME: Offsets greater than 16 bits are not yet implemented.
FIXME: The correct range is a 32 bit sign-extended number.

2975 ↗

(On Diff #103541)

"macro instruction uses large offset, which is not currently supported", also add test for this error.

See test/MC/Mips/macro-la-bad.s for an example of how to write a test that tests an error.

test/MC/Mips/macro-dla-pic.s

1 ↗

(On Diff #103541)

The cpu here should be mips3.

[bsdjhb]

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

2868 ↗

(On Diff #103541)

Do you want me to check for both T9 and T9_64 in the new 64-bit block as well?

[sdardis]

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

2868 ↗

(On Diff #103541)

Please. I'll sit down and refactor this code after it's been committed.

[bsdjhb]

• Revert change to remove T9_64 from O32 CALL16 condition.
• Formatting and other fixes from Simon.
• Add tests for errors for large symbol offsets with dla.
• Check for both T9's in the 64-bit case.

[bsdjhb]

• Wrap long line per clang-format.

[bsdjhb]

test/MC/Mips/macro-dla-pic.s

3 ↗

(On Diff #104399)

Not sure if this comment is really accurate. It was a cut and paste from macro-la-pic.s.

[sdardis]

test/MC/Mips/macro-dla-pic.s

3 ↗

(On Diff #104399)

Yes, it incorrect. Instead of erroring out, a static relocation sequence is written out. Also we don't warn + transform to 'la' instead like GAS does.

[bsdjhb]

• Drop N32 comment from dla-pic test.

[sdardis]

LGTM with a summary update.

[bsdjhb]

I've taken a stab at a new summary. Simon, can you refine that further and commit this change as I do not have commit access?

llvm/trunk/lib/Target/Mips/AsmParser/MipsAsmParser.cpp

return loadImmediate(Offset.getImm(), DstReg, BaseReg, Is32BitAddress, true,
                        IDLoc, Out, STI);
 }
 
 bool MipsAsmParser::loadAndAddSymbolAddress(const MCExpr *SymExpr,
                                             unsigned DstReg, unsigned SrcReg,
                                             bool Is32BitSym, SMLoc IDLoc,
                                             MCStreamer &Out,
                                             const MCSubtargetInfo *STI) {
+  // FIXME: These expansions do not respect -mxgot.
   MipsTargetStreamer &TOut = getTargetStreamer();
   bool UseSrcReg = SrcReg != Mips::NoRegister;
   warnIfNoMacro(IDLoc);
 
   if (inPicMode() && ABI.IsO32()) {
     MCValue Res;
     if (!SymExpr->evaluateAsRelocatable(Res, nullptr, nullptr)) {
       Error(IDLoc, "expected relocatable expression");
if (LoExpr)
                    IDLoc, STI);
 
     if (UseSrcReg)
       TOut.emitRRR(Mips::ADDu, DstReg, TmpReg, SrcReg, IDLoc, STI);
 
     return false;
   }
 
+  if (inPicMode() && ABI.ArePtrs64bit()) {
+    MCValue Res;
+    if (!SymExpr->evaluateAsRelocatable(Res, nullptr, nullptr)) {
+      Error(IDLoc, "expected relocatable expression");
+      return true;
+    }
+    if (Res.getSymB() != nullptr) {
+      Error(IDLoc, "expected relocatable expression with only one symbol");
+      return true;
+    }
+
+    // The case where the result register is $25 is somewhat special. If the
+    // symbol in the final relocation is external and not modified with a
+    // constant then we must use R_MIPS_CALL16 instead of R_MIPS_GOT_DISP.
+    if ((DstReg == Mips::T9 || DstReg == Mips::T9_64) && !UseSrcReg &&
+        Res.getConstant() == 0 &&
+        !(Res.getSymA()->getSymbol().isInSection() ||
+          Res.getSymA()->getSymbol().isTemporary() ||
+          (Res.getSymA()->getSymbol().isELF() &&
+           cast<MCSymbolELF>(Res.getSymA()->getSymbol()).getBinding() ==
+               ELF::STB_LOCAL))) {
+      const MCExpr *CallExpr =
+          MipsMCExpr::create(MipsMCExpr::MEK_GOT_CALL, SymExpr, getContext());
+      TOut.emitRRX(Mips::LD, DstReg, ABI.GetGlobalPtr(),
+                   MCOperand::createExpr(CallExpr), IDLoc, STI);
+      return false;
+    }
+
+    // The remaining cases are:
+    //   Small offset: ld $tmp, %got_disp(symbol)($gp)
+    //                >daddiu $tmp, $tmp, offset
+    //                >daddu $rd, $tmp, $rs
+    // The daddiu's marked with a '>' may be omitted if they are redundant. If
+    // this happens then the last instruction must use $rd as the result
+    // register.
+    const MipsMCExpr *GotExpr = MipsMCExpr::create(MipsMCExpr::MEK_GOT_DISP,
+                                                   Res.getSymA(),
+                                                   getContext());
+    const MCExpr *LoExpr = nullptr;
+    if (Res.getConstant() != 0) {
+      // Symbols fully resolve with just the %got_disp(symbol) but we
+      // must still account for any offset to the symbol for
+      // expressions like symbol+8.
+      LoExpr = MCConstantExpr::create(Res.getConstant(), getContext());
+
+      // FIXME: Offsets greater than 16 bits are not yet implemented.
+      // FIXME: The correct range is a 32-bit sign-extended number.
+      if (Res.getConstant() < -0x8000 || Res.getConstant() > 0x7fff) {
+        Error(IDLoc, "macro instruction uses large offset, which is not "
+                     "currently supported");
+        return true;
+      }
+    }
+
+    unsigned TmpReg = DstReg;
+    if (UseSrcReg &&
+        getContext().getRegisterInfo()->isSuperOrSubRegisterEq(DstReg,
+                                                               SrcReg)) {
+      // If $rs is the same as $rd, we need to use AT.
+      // If it is not available we exit.
+      unsigned ATReg = getATReg(IDLoc);
+      if (!ATReg)
+        return true;
+      TmpReg = ATReg;
+    }
+
+    TOut.emitRRX(Mips::LD, TmpReg, ABI.GetGlobalPtr(),
+                 MCOperand::createExpr(GotExpr), IDLoc, STI);
+
+    if (LoExpr)
+      TOut.emitRRX(Mips::DADDiu, TmpReg, TmpReg, MCOperand::createExpr(LoExpr),
+                   IDLoc, STI);
+
+    if (UseSrcReg)
+      TOut.emitRRR(Mips::DADDu, DstReg, TmpReg, SrcReg, IDLoc, STI);
+
+    return false;
+  }
+
   const MipsMCExpr *HiExpr =
       MipsMCExpr::create(MipsMCExpr::MEK_HI, SymExpr, getContext());
   const MipsMCExpr *LoExpr =
       MipsMCExpr::create(MipsMCExpr::MEK_LO, SymExpr, getContext());
 
   // This is the 64-bit symbol address expansion.
   if (ABI.ArePtrs64bit() && isGP64bit()) {
     // We need AT for the 64-bit expansion in the cases where the optional

llvm/trunk/test/MC/Mips/macro-dla-bad.s

+# RUN: not llvm-mc %s -arch=mips64 -mcpu=mips3 -target-abi n64 2>&1 | \
+# RUN:   FileCheck %s
+
+  .text
+  .option pic2
+  dla $5, symbol+0x8000
+  # CHECK: :[[@LINE-1]]:3: error: macro instruction uses large offset, which is not currently supported
+  dla $5, symbol-0x8001
+  # CHECK: :[[@LINE-1]]:3: error: macro instruction uses large offset, which is not currently supported
+  dla $5, symbol+0x8000($6)
+  # CHECK: :[[@LINE-1]]:3: error: macro instruction uses large offset, which is not currently supported
+  dla $5, symbol-0x8001($6)
+  # CHECK: :[[@LINE-1]]:3: error: macro instruction uses large offset, which is not currently supported
+  dla $25, symbol+0x8000
+  # CHECK: :[[@LINE-1]]:3: error: macro instruction uses large offset, which is not currently supported
+  dla $25, symbol-0x8001
+  # CHECK: :[[@LINE-1]]:3: error: macro instruction uses large offset, which is not currently supported
+  dla $25, symbol+0x8000($6)
+  # CHECK: :[[@LINE-1]]:3: error: macro instruction uses large offset, which is not currently supported
+  dla $25, symbol-0x8001($6)
+  # CHECK: :[[@LINE-1]]:3: error: macro instruction uses large offset, which is not currently supported

llvm/trunk/test/MC/Mips/macro-dla-pic.s

+# RUN: llvm-mc %s -triple=mips64-unknown-linux -show-encoding -mcpu=mips3 | \
+# RUN:   FileCheck %s
+
+.option pic2
+dla $5, symbol        # CHECK: ld $5, %got_disp(symbol)($gp)   # encoding: [0xdf,0x85,A,A]
+                      # CHECK:                                 #   fixup A - offset: 0, value: %got_disp(symbol), kind: fixup_Mips_GOT_DISP
+dla $5, symbol($6)    # CHECK: ld $5, %got_disp(symbol)($gp)   # encoding: [0xdf,0x85,A,A]
+                      # CHECK:                                 #   fixup A - offset: 0, value: %got_disp(symbol), kind: fixup_Mips_GOT_DISP
+                      # CHECK: daddu $5, $5, $6                # encoding: [0x00,0xa6,0x28,0x2d]
+dla $6, symbol($6)    # CHECK: ld $1, %got_disp(symbol)($gp)   # encoding: [0xdf,0x81,A,A]
+                      # CHECK:                                 #   fixup A - offset: 0, value: %got_disp(symbol), kind: fixup_Mips_GOT_DISP
+                      # CHECK: daddu $6, $1, $6                # encoding: [0x00,0x26,0x30,0x2d]
+dla $5, symbol+8      # CHECK: ld $5, %got_disp(symbol)($gp)   # encoding: [0xdf,0x85,A,A]
+                      # CHECK:                                 #   fixup A - offset: 0, value: %got_disp(symbol), kind: fixup_Mips_GOT_DISP
+                      # CHECK: daddiu $5, $5, 8                # encoding: [0x64,0xa5,0x00,0x08]
+dla $5, symbol+8($6)  # CHECK: ld $5, %got_disp(symbol)($gp)   # encoding: [0xdf,0x85,A,A]
+                      # CHECK:                                 #   fixup A - offset: 0, value: %got_disp(symbol), kind: fixup_Mips_GOT_DISP
+                      # CHECK: daddiu $5, $5, 8                # encoding: [0x64,0xa5,0x00,0x08]
+                      # CHECK: daddu $5, $5, $6                # encoding: [0x00,0xa6,0x28,0x2d]
+dla $6, symbol+8($6)  # CHECK: ld $1, %got_disp(symbol)($gp)   # encoding: [0xdf,0x81,A,A]
+                      # CHECK:                                 #   fixup A - offset: 0, value: %got_disp(symbol), kind: fixup_Mips_GOT_DISP
+                      # CHECK: daddiu $1, $1, 8                # encoding: [0x64,0x21,0x00,0x08]
+                      # CHECK: daddu $6, $1, $6                # encoding: [0x00,0x26,0x30,0x2d]
+dla $5, 1f            # CHECK: ld $5, %got_disp(.Ltmp0)($gp)   # encoding: [0xdf,0x85,A,A]
+                      # CHECK:                                 #   fixup A - offset: 0, value: %got_disp(.Ltmp0), kind: fixup_Mips_GOT_DISP
+1:
+
+# PIC expansions involving $25 are special.
+dla $25, symbol       # CHECK: ld $25, %call16(symbol)($gp)     # encoding: [0xdf,0x99,A,A]
+                      # CHECK:                                  #   fixup A - offset: 0, value: %call16(symbol), kind: fixup_Mips_CALL16
+dla $25, symbol($6)   # CHECK: ld $25, %got_disp(symbol)($gp)   # encoding: [0xdf,0x99,A,A]
+                      # CHECK:                                  #   fixup A - offset: 0, value: %got_disp(symbol), kind: fixup_Mips_GOT_DISP
+                      # CHECK: daddu $25, $25, $6               # encoding: [0x03,0x26,0xc8,0x2d]
+dla $25, symbol($25)  # CHECK: ld $1, %got_disp(symbol)($gp)    # encoding: [0xdf,0x81,A,A]
+                      # CHECK:                                  #   fixup A - offset: 0, value: %got_disp(symbol), kind: fixup_Mips_GOT_DISP
+                      # CHECK: daddu $25, $1, $25               # encoding: [0x00,0x39,0xc8,0x2d]
+dla $25, symbol+8     # CHECK: ld $25, %got_disp(symbol)($gp)   # encoding: [0xdf,0x99,A,A]
+                      # CHECK:                                  #   fixup A - offset: 0, value: %got_disp(symbol), kind: fixup_Mips_GOT_DISP
+                      # CHECK: daddiu $25, $25, 8               # encoding: [0x67,0x39,0x00,0x08]
+dla $25, symbol+8($6) # CHECK: ld $25, %got_disp(symbol)($gp)   # encoding: [0xdf,0x99,A,A]
+                      # CHECK:                                  #   fixup A - offset: 0, value: %got_disp(symbol), kind: fixup_Mips_GOT_DISP
+                      # CHECK: daddiu $25, $25, 8               # encoding: [0x67,0x39,0x00,0x08]
+                      # CHECK: daddu $25, $25, $6               # encoding: [0x03,0x26,0xc8,0x2d]
+dla $25, symbol+8($25)# CHECK: ld $1, %got_disp(symbol)($gp)    # encoding: [0xdf,0x81,A,A]
+                      # CHECK:                                  #   fixup A - offset: 0, value: %got_disp(symbol), kind: fixup_Mips_GOT_DISP
+                      # CHECK: daddiu $1, $1, 8                 # encoding: [0x64,0x21,0x00,0x08]
+                      # CHECK: daddu $25, $1, $25               # encoding: [0x00,0x39,0xc8,0x2d]
+dla $25, 1f           # CHECK: ld $25, %got_disp(.Ltmp1)($gp)    # encoding: [0xdf,0x99,A,A]
+                      # CHECK:                                  #   fixup A - offset: 0, value: %got_disp(.Ltmp1), kind: fixup_Mips_GOT_DISP
+1: