[PATCH] D132482: RISCV: permit unaligned nop-slide padding emission

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:365
+    OS.write("\x01\0", 2);
+    Count -= 2;
+  }
----------------
jrtc27 wrote:
> I would suggest not reordering the nop and c.nop in this commit, it's not required. However, if you are going to do it, this is wrong, as if Count is a multiple of 4 this will emit a c.nop, decrease Count by 2 and then emit 1 fewer nop than required.
Also for non-RVC you either need to emit an (illegal) c.nop to pad to a multiple of 4 (which is what binutils does) or emit more zeroes, otherwise you can have exactly the same problem as this patch tries to fix but on non-RVC when `Count % 4 == 3`.


================
Comment at: llvm/test/MC/RISCV/nop-slide.s:1
+# RUN: llvm-mc -triple riscv64 -filetype obj -o - %s | llvm-objdump -d - | FileCheck %s
+
----------------
jrtc27 wrote:
> Should probably be testing both +relax and -relax?
And test +c and -c to ensure both work, that way you'd notice this bug...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132482/new/

https://reviews.llvm.org/D132482