Page MenuHomePhabricator

[RISCV] Update MC compression tests

Authored by sabuasal on Feb 14 2018, 7:54 PM.



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.

Diff Detail


Event Timeline

sabuasal created this revision.Feb 14 2018, 7:54 PM

The patch updates the following tests:

  1. 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.
  2. 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.
  3. 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.
  4. 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).
sabuasal retitled this revision from [WIP, RISCV] Update MC compression tests to [RISCV] Update MC compression tests.Feb 20 2018, 3:43 PM
sabuasal added a subscriber: llvm-commits.
asb added a comment.Feb 27 2018, 1:21 PM

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.

1–13 ↗(On Diff #134370)

You've lost the RELOC checks.

asb added a reviewer: asb.Feb 27 2018, 1:21 PM
asb added inline comments.Feb 27 2018, 1:25 PM
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).

sabuasal added inline comments.Feb 28 2018, 7:29 PM
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).
With relaxation implemented, a compressed branch\jump to an undefined reference will always get relaxed to the expanded form and the relocation generated will be that of the expanded form.

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.

sabuasal marked an inline comment as done.Feb 28 2018, 7:30 PM
asb added inline comments.Mar 1 2018, 12:33 AM
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.

sabuasal added inline comments.Mar 1 2018, 7:37 PM
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.

sabuasal updated this revision to Diff 136673.Mar 1 2018, 7:53 PM

address comments from @asb

moving test update to relevant patch like suggested.

asb accepted this revision.Mar 2 2018, 9:12 AM

Now this is decoupled from the other changes, it seems good to land whenever you're ready.

This revision is now accepted and ready to land.Mar 2 2018, 9:12 AM
This revision was automatically updated to reflect the committed changes.