[PATCH] D33948: Try to handle 'dla' in PIC mode for N64.

[Simon Dardis via Phabricator via llvm-commits]

sdardis requested changes to this revision.
sdardis added a comment.
This revision now requires changes to proceed.

Some small changes requested, but otherwise this looks good.



================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2972-2976
+    if (Res.getConstant() != 0) {
+      // External symbols fully resolve the symbol with just the %got(symbol)
+      // but we must still account for any offset to the symbol for expressions
+      // like symbol+8.
+      LoExpr = MCConstantExpr::create(Res.getConstant(), getContext());
----------------
bsdjhb wrote:
> sdardis wrote:
> > 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.
> 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)
The handling of of a constant and a symbol here is correct for the case of a sign extended 16 bit number.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2868
     // constant then we must use R_MIPS_CALL16 instead of R_MIPS_GOT16.
-    if ((DstReg == Mips::T9 || DstReg == Mips::T9_64) && !UseSrcReg &&
+    if (DstReg == Mips::T9 && !UseSrcReg &&
         Res.getConstant() == 0 && !(Res.getSymA()->getSymbol().isInSection() ||
----------------
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.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2943-2948
+    if (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))) {
----------------
Formatting. See my comment on D33999.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2963-2964
+    // register.
+    const MipsMCExpr *GotExpr =
+        MipsMCExpr::create(MipsMCExpr::MEK_GOT_DISP, Res.getSymA(), getContext());
+    const MCExpr *LoExpr = nullptr;
----------------
Formatting. This should be:

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



================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2972
+
+      // Offsets greater than 16 bits are not yet implemented.
+      if (Res.getConstant() < -0x8000 || Res.getConstant() > 0x7fff) {
----------------
FIXME: Offsets greater than 16 bits are not yet implemented.
FIXME: The correct range is a 32 bit sign-extended number.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2975
+        Error(IDLoc,
+              "pseudo-instruction uses large offset, which is not implemented");
+        return true;
----------------
"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.


================
Comment at: test/MC/Mips/macro-dla-pic.s:1
+# RUN: llvm-mc %s -triple=mips64-unknown-linux -show-encoding -mcpu=mips64r6 | \
+# RUN:   FileCheck %s
----------------
The cpu here should be mips3.


https://reviews.llvm.org/D33948