[nanoMIPS][LLD] nanoMIPS target added to lld #51
Conversation
AndrijaSyrmia
force-pushed
the
andrija/nanomips-llvm16/lld
branch
from
85a3df7
to
e1619d4
Compare
| } | ||
|
|
||
| // TODO: Support for other endianess, and bit size, now it is only Little Endian | ||
| // 32 bit |
There was a problem hiding this comment.
do we have an assert for this?
There was a problem hiding this comment.
There is no assert in NanoMips.cpp itself. However, in getTarget function of lld/ELF/Target.cpp if asked for any other target than 32-bit little endian llvm_unreachable is invoked.
lld/ELF/Arch/NanoMips.cpp
Outdated
| rel.expr), | ||
| bits); | ||
|
|
||
| auto next = it + 1; |
There was a problem hiding this comment.
I think the code in general does not follow https://llvm.org/docs/CodingStandards.html
There was a problem hiding this comment.
The overall code in the whole file, or this instance auto next = it + 1?
AndrijaSyrmia
force-pushed
the
andrija/nanomips-llvm16/lld
branch
from
e1619d4
to
02081f8
Compare
| def GPREL19_S2: RelocProperty32<21, 0xfc000003>; | ||
| def GPREL18: RelocProperty32<18, 0xfc1c0000>; | ||
| def GPREL17_S1: RelocProperty32<18, 0xfc1c0001>; | ||
| def PC10_S1: RelocProperty16HI6<11>; |
There was a problem hiding this comment.
Should R_NANOMIPS_GPREL16_S2 (defined in llvm/include/llvm/BinaryFormat/ELFRelocs/NanoMips.def) define its RelocProperty here like other similar (GPREL) relocation types?
(There seems to be more also missing their definitions, like GPREL18_S3, PC32)
There was a problem hiding this comment.
Well, in general case you are right. But, for now these relocation properties are tied to the linker transformations and since not all relocations are included in transformations those relocations are missing from this file. If a new relocation is used in transformations, than the relocation should be definitely added here. Also, if RelocProperties find another usage other than transforming, other relocs should be added here.
So currently, there are no transformations for R_NANOMIPS_GPREL16_S2 in this implementation, so it isn't present in this file.
There was a problem hiding this comment.
Using the same example, could you elaborate a little bit further on why R_NANOMIPS_GPREL17_S1 is included in transformations but R_NANOMIPS_GPREL16_S2 isn't? Does it also mean that R_NANOMIPS_GPREL16_S2 is currently not generated in object files by assembler? From the document (https://codescape.mips.com/components/toolchain/nanomips/2019.03-01/docs/MIPS_nanoMIPS_ABI_supplement_01_03_DN00179.pdf), looks like both relocation types are used in some instructions.
There was a problem hiding this comment.
Well, there are some reasons why R_NANOMIPS_GPREL16_S2 (as well as R_NANOMIPS_GPREL18_S3) is not included as a RelocProperty. The primary one is that it is not included in gold transformations, which was kind of the base for lld.
The second one is that I didn't manage to generate any of them as the instructions that use these relocations aren't mentioned in the ISA document (https://s3-eu-west-1.amazonaws.com/downloads-mips/I7200/I7200+product+launch/MIPS_nanomips32_ISA_TRM_01_01_MD01247.pdf).
Opposite to these two, there are transformations for R_NANOMIPS_GPREL17_S1 in gold and I managed to generate it through instructions that use this relocation.
Anyways, I think I need to investigate this a little bit more, as this is odd. I will see whether it is possible to generate these instructions and also whether they should be relaxed/expanded. Thank you!
As for R_NANOMIPS_PC32, this is not in RelocProperties, as it is used for data relocations and it is not documented in the ABI supplement. I think it is used in .eh_frame and debug section relocations. The data in these sections won't be expanded/relaxed only the instruction sections.
|
|
||
| const NanoMipsRelocProperty *relocProp = | ||
| relocPropertyTable.getRelocProperty(reloc.type); | ||
| if (!relocProp) |
There was a problem hiding this comment.
For relocation types not expected to appear (like R_NANOMIPS_GPREL16_S2: not expected to be used or not implemented yet), is there any check in case they show up? Is here a good place to add an assert for it?
There was a problem hiding this comment.
There isn't. They are just left as they are (instructions referring to it remain unchanged). This could be a problem if this instruction needs to be expanded, so this will trigger an error later (which is not good).
So either this is a good place (or at the beginning of the for loop) for an assert, or even at the end when actually relocating, if R_NANOMIPS_GPREL16_S2 is not expected at all. Thanks!
AndrijaSyrmia
force-pushed
the
andrija/nanomips-llvm16/lld
branch
from
fa6c8aa
to
446214d
Compare
AndrijaSyrmia
force-pushed
the
andrija/nanomips-llvm16/lld
branch
from
446214d
to
215483b
Compare
Features added: - nanoMIPS relocation resolving - nanoMIPS eflags calculation - .nanoMIPS.abiflags section creation - nanoMIPS _gp symbol setup - special .eh_frame handling in maybeReportUndefined - R_NANOMIPS_PCHI20 renamed to R_NANOMIPS_PC_HI20
AndrijaSyrmia
force-pushed
the
andrija/nanomips-llvm16/lld
branch
from
215483b
to
63f8f51
Compare
Added nanoMIPS specific linker relaxations/expansions: - Transformations are conducted through a driver class NanoMipsTransformController. It scans through relocs of executable input sections and performs relaxations/expansions if possible or necessary - New nanoMIPS specific options used in transformations added to lld - New TableGen backend (NanoMipsTransformationPropertyEmitter.cpp) for emitting properties of instructions and relocations, transformation templates used in linker transformations
Changed parsing of the linker script, as some nanoMIPS linker scripts require different set of rules for parsing: - Dot operator can be used as section address expression, and its value should be determined by the latest value of dot, not the current position in the memory region - Alignment for output section is not ignored for nanoMIPS if output section has got address expression - SHT_NOBITS sections don't occupy load addresses - OVERLAY sections can have a load region and memory region and assignment or byte commands in them.
Changed the way .eh_frame parsing is done for nanoMIPS, as they may contain relocations referring to the size of the CIEs/FDEs. lld reads .eh_frame sections using these sizes so it is necessary that the size value is available at the time of .eh_frame section parsing. It seems like integrated assembler for nanoMIPS is not resolving this relocation at assembly time, but the gnu's assembler for nanoMIPS is resolving it.
Linker relaxation may change relocations (offsets and types). However, when --emit-relocs is used, relocations are simply copied from the input section causing a mismatch with the corresponding (relaxed) code section. This patch fixes this as follows: for non-relocatable RISC-V binaries, `InputSection::copyRelocations` reads relocations from the relocated section's `relocations` array (since this gets updated by the relaxation code). For all other cases, relocations are read from the input section directly as before. In order to reuse as much code as possible, and to keep the diff small, the original `InputSection::copyRelocations` is changed to accept the relocations as a range of `Relocation` objects. This means that, in the general case when reading from the input section, raw relocations need to be converted to `Relocation`s first, which introduces quite a bit of boiler plate. It also means there's a slight code size increase due to the extra instantiations of `copyRelocations` (for both range types). Reviewed By: MaskRay Differential Revision: https://reviews.llvm.org/D159082
Similar to RISC-V relaxations, nanoMIPS transformations change relocations and they need to be updated before writing them down to output. One difference is that nanoMIPS can add new relocations, not just change them like RISC-V, so during linker transformations size of reloc sections need to be updated if --emit-relocs option is on.
AndrijaSyrmia
force-pushed
the
andrija/nanomips-llvm16/lld
branch
from
63f8f51
to
4099547
Compare
|
@jchang-github Hey John, do you maybe have any more comments/suggestions on lld? I've added some more tests as well to test the coverage later. There's also work left in reformatting the code. |
Support for nanoMIPS little endian 32bit target added to lld