This patch updates MC tests related to compression in RISCV to
insure they work correctly with automatic compression and relaxation
enabled. This is the first part of a series of patches to implement
automatic compression for RISCV.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
The patch updates the following tests:
- test/MC/RISCV/cnop.s The test checks for generation of c.nop for alignment. With automatic compression the instruction 'addi sp, sp, -16' will be compressed to c.addi, this will require the MC layer to insert 4 bytes instead of 2 to insure 8 byte alignment. RISCVAsmBackend::writeNopData() will write one 4 byte nop instruction 'addi, x0, x0, 0' instead of the 2 byte c.nop it needed before. Updated the test to replace the add with two separate c.addi to force the generation of c.nop with automatic compression.
- test/MC/RISCV/fixups-compressed.s: The test contains a compressed jump to an undefined symbol (func1). If this jump is to be relaxed in the MC layer it will be transformed to an expanded jump to provide maximum range (look at MCAsmBackend::fixupNeedsRelaxationAdvanced()). Updated the test to make the symbol resolved.
- test/MC/RISCV/rv32c-only-valid.s In r320797 The flag '-riscv-no-aliases' was mistakenly passed to the llvm-mc run line instead of the llvm-objdump piped to it, this will cause the expanded version of the instruction to be printed.
- test/MC/RISCV/relocations.s The test 'jal zero, foo' will get compressed to c.jal foo and the relocation rvc_jump will be generated instead of R_RISCV_JAL. Split the test into two files, one for relocations of expanded instructions and (relocatoins.s) with compression turned disabled, and another for compressed instruction relocation (compress-relocations).
If you explicitly add me as a reviewer for any RISC-V patches it's easier for me to see it on my TODO list. It looks like the stack of dependent patches is incomplete here - filling that out makes it a little easier to keep track of what's reviewed/committed etc.
Minor issue with compress-relocations.s, but otherwise seems sensible. I'd hold off committing until the compression patch is reviewed, primarily because suggestions to change testing in that patch might mean you'd want to come back and edit these changes.
The issue with fixups-compressed.s is a good spot, the best fix would be to update that so there's a FileCheck line that ensures no relocations are generated (similar to fixups.s). That fix could be committed directly, as that's a straight-forward improvement that makes sense regardless of any instruction compression patches.
test/MC/RISCV/compress-relocations.s | ||
---|---|---|
1–13 ↗ | (On Diff #134370) | You've lost the RELOC checks. |
test/MC/RISCV/compress-relocations.s | ||
---|---|---|
1–13 ↗ | (On Diff #134370) | Also if we're going to separate this to a separate file, relocations-compressed.s would be more consistent with naming of other files (such as fixups-compressed.s). |
test/MC/RISCV/compress-relocations.s | ||
---|---|---|
1–13 ↗ | (On Diff #134370) | I actually didn't forget. This is a bit tricky. MC for RISCV , as of now, only generates relocations if the target for the branch\jump is unresolved. MC also relaxes branches\jumps to unresolved targets (look at MCAssembler::handleFixup and MCAssembler::fixupNeedsRelaxation). One way this could be circumvented (if we need to) is by implementing shouldForceRelocation, this way a compressed branch to a local (resolved) symbol will also get a relocation. |
test/MC/RISCV/compress-relocations.s | ||
---|---|---|
1–13 ↗ | (On Diff #134370) | I think the correct thing to do is to leave the RELOC checks in this patch, then update the checks in the patch that actually changes the behaviour. |
test/MC/RISCV/compress-relocations.s | ||
---|---|---|
1–13 ↗ | (On Diff #134370) | I agree. Actually I think I'll take your advice on this and on holding off committing this patch till after the main compression patches are reviewed. I'll update the dependencies accordingly. I integrated the change we need to the relocation to the relaxation patch (D43055) to make it easier to trace (updating the relocation in relocations.s and adding a func label in fixups-comperessed with checking for not generating a relocation). I will not separate the relocations.s file for now file for now until we figure out the other patches first. |
Now this is decoupled from the other changes, it seems good to land whenever you're ready.