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

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

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.



================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:95-96
+                                           const MCAsmLayout &Layout) const {
+  // FIXME: Do we need to subtract 2 from the value we receive to
+  // account for the PC advancement.
+  int64_t Offset = int64_t(Value);
----------------
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.


================
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:
----------------
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
```


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


https://reviews.llvm.org/D43055