It is the same as LA, except that it can also load 64-bit addresses and it only works on 64-bit MIPS architectures.
Details
Details
Diff Detail
Diff Detail
Event Timeline
| lib/Target/Mips/AsmParser/MipsAsmParser.cpp | ||
|---|---|---|
| 880–882 | This should test for N64, preferably using MipsABIInfo::ArePtrs64bit() | |
| 2064–2070 | Similarly, these two isGP64bit() checks should be checks for 64-bit pointers via the MipsABIInfo class | |
| 2088–2089 | This comment is only correct for the 'if (UseSrcReg && (SrcReg == DstReg))' path. Could you move/update it to cover the other path? | |
| 2116 | 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. | |
| 2117 | Nit: Blank line | |
| 2144–2153 | (Phabricator won't let me delete this comment) | |
Comment Actions
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.
Comment Actions
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 | ||
|---|---|---|
| 880–882 | Fixed. | |
| 2064–2070 | The first one was fixed in a separate patch. | |
| 2088–2089 | I had to rewrite the comments anyway, so it's been fixed. | |
| 2116 | Regarding the first sub-comment, it's true that GAS expands those differently, Regarding the second sub-comment, the LD + GOT_DISP relocation expansion is As for -mno-abicalls, that seems more of a driver issue to me. AFAICT gcc with -m-abicalls passes -KPIC to gas while gcc + -mno-abicalls does not. | |
| 2117 | Fixed. | |
Comment Actions
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.
Comment Actions
LGTM with a small test case as described in the comments below.
| lib/Target/Mips/AsmParser/MipsAsmParser.cpp | ||
|---|---|---|
| 1845 | Nit: Parenthesize the expression. | |
| 1937 | Nit: emitRI expects an int16_t which leads to an "overflow in implicit constant conversion" warning due to the hexadecimal literal. | |
| 2051–2055 | 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 | Nit: Delete "is". | |
Comment Actions
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/buildComment Actions
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 | dla fails for N32 because dla is intended for 64-bit pointers. I believe N32 should use la. Do you agree? | |
| lib/Target/Mips/AsmParser/MipsAsmParser.cpp | ||
|---|---|---|
| 2051–2055 | Yes, that makes sense. You can go ahead and commit this. | |
Comment Actions
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.
This should test for N64, preferably using MipsABIInfo::ArePtrs64bit()