[PATCH] D75203: Relax existing instructions to reduce the number of nops needed for alignment purposes

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/MC/MCAssembler.cpp:1186
+
+  for (iterator it = begin(), ie = end(); it != ie; ++it) {
+    MCSection &Sec = *it;
----------------
skan wrote:
> Capitalize the first character of the variable it
`for (MCSection &Sec : *this) {`


================
Comment at: llvm/lib/MC/MCAssembler.cpp:1192
+    SmallVector<MCRelaxableFragment*, 4> Relaxable;
+    for (MCSection::iterator I = Sec.begin(), IE = Sec.end(); I != IE; ++I) {
+      MCFragment &F = *I;
----------------
`for (MCFragment &F : Sec) {`


================
Comment at: llvm/lib/MC/MCAssembler.cpp:1198-1200
+      if (F.getKind() ==  MCFragment::FT_Data)
+        // Skip and ignore
+        continue;
----------------
skan wrote:
> `F.getKind() ==  MCFragment::FT_Data || F.getKind() == FT_CompactEncodedInst`
> I think we can handle `MCCompactEncodedInstFragment` here.
It seems we can use `switch (F.getKind())` here.


================
Comment at: llvm/lib/MC/MCAssembler.cpp:1237
+        if (getBackend().mayNeedRelaxation(RF.getInst(),
+                                           *RF.getSubtargetInfo())) {
+          // If we have an instruction which hasn't been fully relaxed, we
----------------
The comment can be moved before the `if`. Then the brace can be deleted.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:656
+  MCInst Relaxed;
+  relaxInstruction(RF.getInst(), *RF.getSubtargetInfo(), Relaxed);
+
----------------
`*RF.getSubtargetInfo()` -> `STI` (it is a member variable)


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:659
+  SmallVector<MCFixup, 4> Fixups;
+  SmallString<256> Code;
+  raw_svector_ostream VecOS(Code);
----------------
`SmallString<16>` should be sufficient. (The SmallString used in MCAssembler::relaxInstruction should be fixed instead)


================
Comment at: llvm/test/MC/X86/align-via-relaxation.s:1
+  # RUN: llvm-mc -mcpu=skylake -filetype=obj -triple x86_64-pc-linux-gnu %s | llvm-objdump -d --section=.text - | FileCheck %s
+
----------------
The `RUN` line is unnecessarily indented. It is not common.

`-pc-linux-gnu` can be deleted to make the line length smaller.


================
Comment at: llvm/test/MC/X86/align-via-relaxation.s:6
+	.text
+	.section	.text
+  # Demonstrate that we can relax instructions to provide padding, not
----------------
`.text` and `.section .text` do the same thing. We can actually delete `.file`, `.text` and `.section .text`.


================
Comment at: llvm/test/MC/X86/align-via-relaxation.s:10
+  # CHECK: .text
+  # CHECK: 0: eb 1f                        	jmp 31 <foo>
+  # CHECK: 2: e9 1a 00 00 00               	jmp 26 <foo>
----------------
`jmp` is misaligned.


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

https://reviews.llvm.org/D75203