Page MenuHomePhabricator

[mips] [IAS] Add support for the DLA pseudo-instruction and fix problems with DLI
ClosedPublic

Authored by dsanders on May 6 2015, 5:11 AM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

emaste added a subscriber: emaste.
dsanders requested changes to this revision.May 13 2015, 5:50 AM
dsanders edited edge metadata.
dsanders added inline comments.
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
821 ↗(On Diff #25038)

This should test for N64, preferably using MipsABIInfo::ArePtrs64bit()

1959–1963 ↗(On Diff #25038)

Similarly, these two isGP64bit() checks should be checks for 64-bit pointers via the MipsABIInfo class

1994–1999 ↗(On Diff #25038)

This comment is only correct for the 'if (UseSrcReg && (SrcReg == DstReg))' path. Could you move/update it to cover the other path?

2026 ↗(On Diff #25038)

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.

2027 ↗(On Diff #25038)

Nit: Blank line

2054–2063 ↗(On Diff #25038)

(Phabricator won't let me delete this comment)

This revision now requires changes to proceed.May 13 2015, 5:50 AM
tomatabacu edited edge metadata.

Rebased (for Sean Bruno's convenience).

tomatabacu planned changes to this revision.Jun 11 2015, 10:35 AM

Will address review comments and other issues soon.

tomatabacu updated this revision to Diff 28105.Jun 22 2015, 6:09 AM
tomatabacu edited edge metadata.

Addressed review comments.
Changed behaviour to more closely match GAS.
Refactored if-else control flow to early returns.
Added test cases for DLA with $at as an operand.
Added more test cases for unusual behaviours of (D)LA.
Added test cases for mips-expansions-bad.s.
Removed some unnecessary generation checks to fix mips-expansions-bad.s.
Removed unnecessary patch dependecies.
Rebased on top of D10568.

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).

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
821 ↗(On Diff #25038)

Fixed.

1959–1963 ↗(On Diff #25038)

The first one was fixed in a separate patch.
The second one is more consistent with GAS' behaviour.

1994–1999 ↗(On Diff #25038)

I had to rewrite the comments anyway, so it's been fixed.

2026 ↗(On Diff #25038)

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.

2027 ↗(On Diff #25038)

Fixed.

dsanders commandeered this revision.Jul 2 2015, 3:05 AM
dsanders edited reviewers, added: tomatabacu; removed: dsanders.

Commandeered this patch since Toma finished his placement with us last Friday.

dsanders updated this revision to Diff 29905.Jul 16 2015, 7:46 AM

Fixed several bugs as well as differences in output compared to GAS.

At this point, li/la/dli/dla produce the same output as GAS except for:

  • N32 emits ELF64. This is a known bug.
  • Relocs are based on local symbols rather than .text. Another known bug.
  • la does not promote to dla on N64 with a warning. It errors instead. This is due to GPR32 vs GPR64 mismatches.
dsanders retitled this revision from [mips] [IAS] Add support for the DLA pseudo-instruction. to [mips] [IAS] Add support for the DLA pseudo-instruction and fix problems with DLI.Jul 16 2015, 7:57 AM
brooks added a subscriber: brooks.Jul 21 2015, 2:07 PM
vkalintiris edited edge metadata.Jul 22 2015, 4:42 AM

LGTM with a small test case as described in the comments below.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
1845 ↗(On Diff #29905)

Nit: Parenthesize the expression.

1937 ↗(On Diff #29905)

Nit: emitRI expects an int16_t which leads to an "overflow in implicit constant conversion" warning due to the hexadecimal literal.

2051–2055 ↗(On Diff #29905)

Could you add a test for this case? Maybe in macro-la-bad.s?

Also, the following test case currently fails for N32:

dla $5, 0x000000000($6)
test/MC/Mips/macro-la.s
10 ↗(On Diff #29905)

Nit: Delete "is".

seanbruno requested changes to this revision.Aug 16 2015, 4:27 PM
seanbruno added a reviewer: seanbruno.

This needs to be updated due to code rot. It applies cleanly to head, however it does not compile tue to changes causing the following compile failures:

[ 40%] Building CXX object lib/Target/Mips/MCTargetDesc/CMakeFiles/LLVMMipsDesc.dir/MipsABIInfo.cpp.o
/home/sbruno/clang/llvm/lib/Target/Mips/MCTargetDesc/MipsABIInfo.cpp:110:23: error: out-of-line definition of 'GetZeroReg' does not match any declaration in 'llvm::MipsABIInfo'
unsigned MipsABIInfo::GetZeroReg() const {
                    ^~~~~~~~~~
/home/sbruno/clang/llvm/lib/Target/Mips/MCTargetDesc/MipsABIInfo.cpp:111:10: error: use of undeclared identifier 'AreGprs64bit'; did you mean 'ArePtrs64bit'?
return AreGprs64bit() ? Mips::ZERO_64 : Mips::ZERO;
       ^~~~~~~~~~~~
       ArePtrs64bit
/home/sbruno/clang/llvm/lib/Target/Mips/MCTargetDesc/MipsABIInfo.h:73:15: note: 'ArePtrs64bit' declared here
inline bool ArePtrs64bit() const { return IsN64(); }
            ^
2 errors generated.
*** Error code 1

Stop.
make[2]: stopped in /usr/home/sbruno/clang/build
*** Error code 1

Stop.
make[1]: stopped in /usr/home/sbruno/clang/build
*** Error code 1

Stop.
make: stopped in /usr/home/sbruno/clang/build
This revision now requires changes to proceed.Aug 16 2015, 4:27 PM

It didn't apply cleanly for me and when I resolve the patch application issues it works for me. Assuming Vasileios agrees with the question below, I'll commit.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
2051–2055 ↗(On Diff #29905)

dla fails for N32 because dla is intended for 64-bit pointers. I believe N32 should use la. Do you agree?

vkalintiris accepted this revision.Aug 17 2015, 3:10 AM
vkalintiris edited edge metadata.
vkalintiris added inline comments.
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
2051–2055 ↗(On Diff #29905)

Yes, that makes sense. You can go ahead and commit this.

This revision was automatically updated to reflect the committed changes.

The nits are fixed in the commit. I was going to refresh the patch beforehand but it seems arcanist no longer allows that for svn.