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

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd marked 3 inline comments as done.
compnerd added a comment.

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.



================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:366
+  // section (otherwise we have unaligned instructions, and thus have far
+  // bigger problems), so just write zeros instead.  This also may occur for
+  // non-standard data sections which are not declared and thus get detected as
----------------
jrtc27 wrote:
> IALIGN; C changes this from 4 to 2
Sorry, I don't understand what you mean by `IALIGN`.  Yeah, I should fix that to say "4-byte  (or 2-byte under C)".


================
Comment at: llvm/test/MC/RISCV/nop-slide.ll:1
+; RUN: llc -mtriple riscv64-unknown-linux-gnu -filetype obj -o - %s | llvm-objdump -t - | FileCheck %s
+
----------------
jrtc27 wrote:
> Just riscv64, not Linux-specific
:shrug: sure.


================
Comment at: llvm/test/MC/RISCV/nop-slide.ll:4
+target datalayout = "e-m:e-p:64:64-i64:64-i128:128-n64-S128"
+target triple = "riscv64-unknown-linux-gnu"
+
----------------
jrtc27 wrote:
> These should be redundant
Agreed.  Removed.


================
Comment at: llvm/test/MC/RISCV/nop-slide.ll:7
+ at b = dso_local global i8 0, section ".init.data", align 1
+ at c= dso_local global ptr null, section ".init.data", align 8
+
----------------
jrtc27 wrote:
> How does this test it? There’s no code here to align?
Correct, it is purely data, but the section is getting classified as text (as is the behaviour with gas).   The other architectures already properly handled this, but RISC-V did not.  The result was that the alignment of `c` required that we emit 7-bytes of nops to align, and that would fail.  Added a comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132482