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

Tue Aug 23 10:57:42 PDT 2022

jrtc27 added a comment.

In D132482#3743100 <https://reviews.llvm.org/D132482#3743100>, @compnerd wrote:

> binutils's behaviour is the opposite of what we have:
>
> 1. 1 byte nop to align to 2.
> 2. At most 1 2-byte nop sequence.
> 3. Pad via 4-byte nop sequence.
>
> I don't have a strong opinion on this, this is going to be identical in practice due to the modular nature.

Modular nature? I'd prefer to do what binutils does wrt where to put the 0 (I don't care at which end the c.nop goes, //that// really doesn't matter), it makes more sense and is less likely to confuse tools, even if in practice nothing should be executing any of these bytes.

================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:363
+      OS.write("\x01\0", 2);
+
+  // If the count is not 4-byte aligned (or 2-byte under the C extensions), we
----------------
Making this a loop is a bit odd when we know it's always executing 0 or 1 times

================
Comment at: llvm/test/MC/RISCV/nop-slide.s:1
+# RUN: llvm-mc -triple riscv64 -filetype obj -o - %s | llvm-objdump -d - | FileCheck %s
+
----------------
Should probably be testing both +relax and -relax?

================
Comment at: llvm/test/MC/RISCV/nop-slide.s:3-7
+	.balign 4
+	.byte 0
+
+	.balign 4
+	auipc a0, 0
----------------
Why indent? Our tests don't do so unless there are labels involved as it just looks weird otherwise.

================
Comment at: llvm/test/MC/RISCV/nop-slide.s:10-11
+# CHECK: 0000000000000000 <.text>:
+# CHECK-NEXT: 0: 00 00        	<unknown>
+# CHECK-NEXT: 2: 00 00        	<unknown>
+# CHECK-NEXT: 4: 17 05 00 00  	auipc	a0, 0
----------------
This test is more interesting if you enable C and/or bump up the alignment as currently it's just padding with 3 zeroes. Maybe do both so you can clearly see the 0, c.nop and nop in the output?

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

https://reviews.llvm.org/D132482