[PATCH] D97982: [MC] Introduce NeverAlign fragment type

[Kan Shengchen via Phabricator via llvm-commits]

skan added inline comments.


================
Comment at: llvm/include/llvm/MC/MCFragment.h:343-350
+  /// target dependent.
+  bool EmitNops : 1;
+
+  /// Value to use for filling padding bytes.
+  int64_t Value;
+
+  /// The size of the integer (in bytes) of \p Value.
----------------
If MCNeverAlignFragment only emits nop in your usage scenarios, the members EmitNops, Value and ValueSize can be removed.  Otherwise, there may be a lot of untested code. 


================
Comment at: llvm/include/llvm/MC/MCFragment.h:360-365
+  int64_t getValue() const { return Value; }
+
+  unsigned getValueSize() const { return ValueSize; }
+
+  bool hasEmitNops() const { return EmitNops; }
+  void setEmitNops(bool Value) { EmitNops = Value; }
----------------
Remove these interfaces.


================
Comment at: llvm/lib/MC/MCAssembler.cpp:365-367
+      assert(getBackend().getMinimumNopSize() != NAF.getAlignment());
+      return getBackend().getMinimumNopSize();
+    }
----------------
The summary and comments in the code need to be updated, you insert a minimum-size nop rather than a single one-byte nop, although they are same for most cases.


================
Comment at: llvm/lib/MC/MCAssembler.cpp:611-629
+    for (uint64_t I = 0; I != Count; ++I) {
+      switch (NAF.getValueSize()) {
+      default:
+        llvm_unreachable("Invalid size!");
+      case 1:
+        OS << char(NAF.getValue());
+        break;
----------------
Line 611-628 are redundant and untested.


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:4743-4750
+  if (getParser().parseToken(
+          AsmToken::EndOfStatement,
+          "unexpected token in '.avoid_end_align' directive"))
+    return true;
+
+  if (Alignment <= 0) {
+    Error(AlignmentLoc, "'.avoid_end_align' directive with non-positive size");
----------------
Could you add test cases for line 4743-4750?


================
Comment at: llvm/test/MC/X86/x86_64-directive-avoid_end_align.s:1
+# RUN: llvm-mc -triple=x86_64 %s -filetype=obj | llvm-objdump -d - | FileCheck %s
+
----------------
Add "--no-show-raw-insn" after llvm-objdump since you do not need to check instruction encoding. 


================
Comment at: llvm/test/MC/X86/x86_64-directive-avoid_end_align.s:5-6
+.nops 59
+  pushq %rbx
+  movl  %edi, %ebx
+.avoid_end_align 64
----------------
If you remove line 5-6 and change ".nops 59" to ".nops 62", the purpose of the test can be more clear. And you need to check there is the extra nop is inserted **only**  when cmp ends and jmp starts exactly at 64b alignment.


================
Comment at: llvm/test/MC/X86/x86_64-directive-avoid_end_align.s:13-16
+  nop
+.LBB0_2:                                # %untaken
+  popq  %rbx
+  retq
----------------
Line 13-16 can be removed.


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

https://reviews.llvm.org/D97982