[PATCH] D97982: [MC] Introduce NeverAlign fragment type
Kan Shengchen via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 28 01:52:47 PDT 2021
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
More information about the llvm-commits
mailing list