[PATCH] [mips] [IAS] Add support for the DLA pseudo-instruction.

Toma Tabacu toma.tabacu at imgtec.com
Mon Jun 22 06:13:28 PDT 2015

Right now, there still are some missing pieces:

The mips-expansions-bad.s test cases which involve (D)LA of an immediate do not currently work.
They will require further changes, mostly in expandLoadAddressImm() and expandLoadAddressReg().

There are no DLA test cases in set-nomacro.s (only for LA).

Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:821
@@ -820,2 +820,3 @@
-    Inst.addOperand(MCOperand::CreateReg(getMemBase()->getGPR32Reg()));
+    Inst.addOperand(MCOperand::CreateReg(AsmParser.isGP64bit()
+                                             ? getMemBase()->getGPR64Reg()
dsanders wrote:
> This should test for N64, preferably using MipsABIInfo::ArePtrs64bit()

Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1959-1963
@@ -1950,5 +1958,7 @@
                                       SmallVectorImpl<MCInst> &Instructions) {
   if (Is32BitSym && isGP64bit())
             "instruction loads a 32-bit address on a 64-bit architecture");
+  if (!Is32BitSym && !isGP64bit()) {
+    Error(IDLoc, "instruction requires a 64-bit architecture");
dsanders wrote:
> Similarly, these two isGP64bit() checks should be checks for 64-bit pointers via the MipsABIInfo class
The first one was fixed in a separate patch.
The second one is more consistent with GAS' behaviour.

Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1994-1999
@@ -1980,8 +1993,8 @@
     // If it's a 64-bit architecture, expand to:
     // la d,sym => lui  d,highest(sym)
     //             ori  d,d,higher(sym)
     //             dsll d,d,16
     //             ori  d,d,hi16(sym)
     //             dsll d,d,16
     //             ori  d,d,lo16(sym)
     const MCExpr *HighestExpr = evaluateRelocExpr(SymOp->getExpr(), "highest");
dsanders wrote:
> This comment is only correct for the 'if (UseSrcReg && (SrcReg == DstReg))' path. Could you move/update it to cover the other path?
I had to rewrite the comments anyway, so it's been fixed.

Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2026
@@ +2025,3 @@
+      return false;
+    } else if (!UseSrcReg || (UseSrcReg && (SrcReg != DstReg))) {
dsanders wrote:
> Strangely, GAS only seems to expand this way for:
>   dla $2, foo($3)
> and not:
>   dla $2, 0x0001000200030004($3)
> which generates the longer code above. I think we should mention this to them.
> Also, I don't get this expansion when running this instruction through the GCC driver:
>   dla $2, sym($8)
> Instead I get:
>   8c:	df820000 	ld	v0,0(gp)
> 			8c: R_MIPS_GOT_DISP	.text
> 			8c: R_MIPS_NONE	*ABS*
> 			8c: R_MIPS_NONE	*ABS*
>   90:	0048102d 	daddu	v0,v0,a4
> I think it needs to depend on -mno-abicalls.
Regarding the first sub-comment, it's true that GAS expands those differently,
but so do we, as the case without the symbol goes through a different code path than
the one highlighted here (it's expanded by loadImmediate() not loadAndAddSymbolAddress()).

Regarding the second sub-comment, the LD + GOT_DISP relocation expansion is
supposed to happen when PIC mode is enabled. I was not intending to add support
for PIC mode DLA with this patch.

As for -mno-abicalls, that seems more of a driver issue to me.
I don't think the assembler is supposed to know what abicalls means.

AFAICT gcc with -m-abicalls passes -KPIC to gas while gcc + -mno-abicalls does not.
At the moment, clang always passes -KPIC, regardless of the abicalls option, so
that might be the problem.

Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2027
@@ +2026,3 @@
+    } else if (!UseSrcReg || (UseSrcReg && (SrcReg != DstReg))) {
+      // This only happens if !UseSrcReg.
dsanders wrote:
> Nit: Blank line



More information about the llvm-commits mailing list