[PATCH] D47755: [RISCV] Insert R_RISCV_ALIGN relocation type and Nops for code alignment when linker relaxation enabled

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

@shiva0217 the alignment required will be satisfied, but the linker may overwrite some of the bytes with nop/c.nop which seems undesirable.

I added a few comments. Have you looked much at what gas does for .align in other sections. Maybe it's error on my part but I'm not sure that it's doing the right thing...



================
Comment at: include/llvm/MC/MCAsmBackend.h:91
 
+  /// Hook for the target need to insert extra Nops for alignment directive.
+  virtual bool
----------------
How about: "Hook to check if extra nop bytes must be inserted for alignment directive. For some targets this may be necessary in order to support linker relaxation. The number of bytes to insert are returned in Size."


================
Comment at: include/llvm/MC/MCAsmBackend.h:93
+  virtual bool
+  NopBytesToInsertForAlignDirectiveInCodeSection(const MCAlignFragment &AF,
+                                                 unsigned &Size) {
----------------
Should start with a lower-case letter.

Might be better named "shouldInsterExtraNopBytesForCodeAlign"? The use of CodeAlign aligns with the way the term is used in MCSection.h for UseCodeAlign.


================
Comment at: include/llvm/MC/MCAsmBackend.h:98
+
+  /// Hook for the target need to insert fixup type for alignment directive.
+  virtual bool insertFixupForAlignDirectiveInCodeSection(
----------------
How about "Hook which indicates if the target requires a fixup to be generated when handling an align directive in an executable section (as determined by MCSection::UseCodeAlign())."


================
Comment at: include/llvm/MC/MCAsmBackend.h:99
+  /// Hook for the target need to insert fixup type for alignment directive.
+  virtual bool insertFixupForAlignDirectiveInCodeSection(
+      MCAssembler &Asm, const MCAsmLayout &Layout, MCAlignFragment &AF) {
----------------
Maybe "shouldInsertFixupForCodeAlign"?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D47755