This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Implement MC relaxations for compressed instructions.
ClosedPublic

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

Details

Summary
This patch implements relaxation for RISCV in the MC layer.
 The following relaxations are currently handled:
 1) Relax C_BEQZ to BEQ and C_BNEZ to BNEZ in RISCV.
 2) Relax and C_J $imm  to JAL x0, $imm  and CJAL to JAL ra, $imm.

Diff Detail

Repository
rL LLVM

Event Timeline

sabuasal created this revision.Feb 7 2018, 5:14 PM
sabuasal updated this revision to Diff 134375.Feb 14 2018, 8:07 PM
sabuasal edited the summary of this revision. (Show Details)
sabuasal removed subscribers: asb, rbar, johnrusso and 5 others.
sabuasal retitled this revision from [WIP][RISCV] Implement MC relaxations for compressed instructions. to [RISCV] Implement MC relaxations for compressed instructions..Feb 20 2018, 2:55 PM
sabuasal added a reviewer: asb.
asb added a comment.Feb 27 2018, 3:09 PM

Is it possible to add tests for this?

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
118 ↗(On Diff #134375)

It would be clearer to just have the comment summarise the transform. e.g.:
// c.beqz $rs1, $imm -> beq $rs1, x0, $imm

And similar for other cases

153–158 ↗(On Diff #134375)

Surely these (unsigned) casts are unnecessary?

sabuasal updated this revision to Diff 136458.Feb 28 2018, 7:11 PM
sabuasal marked 2 inline comments as done.

Address comments from Alex!

sabuasal updated this revision to Diff 136459.Feb 28 2018, 7:12 PM

Address comments from Alex.

Address comments from @asb .

asb added a comment.Mar 1 2018, 2:37 AM

Thanks for adding the test case, I added a few minor comments there.

After resolving those, the only remaining issue would be the lack of test coverage for negative offsets.

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
95–96 ↗(On Diff #136459)

Are you happy to remove this FIXME? Looking at the test case, it seems to me that the correct offset is generated. Please take a look at the objdump and check you agree.

test/MC/RISCV/relaxation.s
1–2 ↗(On Diff #136459)

Using pipes here is a more user-friendly, as in the case of a test failure you get a nicer command to copy and paste that doesn't produce unwanted temporaries. Given we expect this to work for riscv64 as well, add a RUN line for that too:

# RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+c < %s \
# RUN:     | llvm-objdump -d - | FileCheck -check-prefix=INSTR %s
# RUN: llvm-mc -filetype=obj -triple riscv64 -mattr=+c < %s \
# RUN:     | llvm-objdump -d - | FileCheck -check-prefix=INSTR %s
5 ↗(On Diff #136459)

I think checking the instruction location is unnecessary (and a little brittle) as you're already using #INSTR-NEXT. You should include all instruction operands in the check line as that demonstrates that the fixup has been resolved to a sensible value.

e.g.
#INSTR: c.bnez a0, 56

sabuasal updated this revision to Diff 136671.Mar 1 2018, 7:31 PM
sabuasal marked an inline comment as done.
sabuasal added inline comments.
test/MC/RISCV/relaxation.s
1–2 ↗(On Diff #136459)

c.jal is an RV32C instruction only, so I think I have to break this test into two tests. One for 32 and one for 64 bits.

5 ↗(On Diff #136459)

I added the location because I wanted to check for the instruction size (2 or 4). But I think you are right, it is enough to check the mnemonic for that and check all the operands.

sabuasal marked 4 inline comments as done.Mar 1 2018, 7:33 PM
asb accepted this revision.Mar 2 2018, 9:17 AM

Looks good to me. You might want to use c.nop in the tests rather than nop to avoid test churn (changing offsets) once automatic compression is merged.

This revision is now accepted and ready to land.Mar 2 2018, 9:17 AM
This revision was automatically updated to reflect the committed changes.
sabuasal added a comment.EditedMar 2 2018, 2:13 PM

Thanks for the timely review, Alex!

In D43055#1025496, @asb wrote:

Looks good to me. You might want to use c.nop in the tests rather than nop to avoid test churn (changing offsets) once automatic compression is merged.

Updated the nop to c.nop in the committed version.