[PATCH] D43055: [RISCV] Implement MC relaxations for compressed instructions.

Sameer AbuAsal via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Mar 1 19:32:00 PST 2018


sabuasal added inline comments.


================
Comment at: test/MC/RISCV/relaxation.s:1-2
+# RUN: llvm-mc -triple riscv32 -mattr=+c -filetype=obj %s -o %t1
+# RUN: llvm-objdump -d %t1 | FileCheck -check-prefix=INSTR %s
+start:
----------------
asb wrote:
> 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
> ```
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.


================
Comment at: test/MC/RISCV/relaxation.s:5
+  c.bnez a0, NEAR
+#INSTR: 0: {{.*}} c.bnez a0
+  c.beqz a0, NEAR
----------------
asb wrote:
> 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`
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.


https://reviews.llvm.org/D43055





More information about the llvm-commits mailing list