[PATCH] D72225: Align branches within 32-Byte boundary(Prefix padding)

[Kan Shengchen via Phabricator via llvm-commits]

skan added inline comments.


================
Comment at: llvm/lib/MC/MCAssembler.cpp:1009
+    AlignedSize += Size;
+    AlignedOffset -= Size;
   }
----------------
MaskRay wrote:
> `AlignedOffset` can be defined after AlignedSize is computed.
Sorry, I didn't get your point.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:80
     "x86-align-branch-boundary", cl::init(0),
-    cl::desc(
-        "Control how the assembler should align branches with NOP. If the "
-        "boundary's size is not 0, it should be a power of 2 and no less "
-        "than 32. Branches will be aligned within the boundary of specified "
-        "size. -x86-align-branch-boundary=0 doesn't align branches."));
+    cl::desc("Control how the assembler should align branches with NOP or "
+             "segment override prefix. If the boundary's size is not 0, it "
----------------
reames wrote:
> a) segment prefixes aren't the only ones used are they?
> b) the wording changes can be pulled into their own review (or simply committed)
If there is no room to insert prefix, NOP will be emitted before the branch or fused pair.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:166
     AlignBranchType = X86AlignBranchKindLoc;
+    AlignMaxPrefixSize = std::min<uint8_t>(X86AlignBranchPrefixSize, 5);
   }
----------------
MaskRay wrote:
> `AlignMaxPrefixSize = X86AlignBranchPrefixSize;`
> 
> The error checking and normalization (to 5) should be done in an earlier place.
Do you any suggestions about a appropriate, earlier place?


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:498
+      case X86::CS:
+        return 0x2e;
+      case X86::SS:
----------------
reames wrote:
> Please define an enum which gives a symbolic name for these values (if there isn't already one).  (i.e. what the heck are these integer constant values?)
It is the exact value to be emitted when it has corresponding segment prefix. See `X86MCCodeEmitter::emitSegmentOverridePrefix`


================
Comment at: llvm/test/MC/X86/align-branch-64-8a.s:1
+# Check the case multiple CMPs are followed a jcc is correctly handled.
+# RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown --x86-align-branch-boundary=32 --x86-align-branch=fused+jcc  --x86-align-branch-prefix-size=5 %s | llvm-objdump -d  - | FileCheck %s
----------------
MaskRay wrote:
> Use `## ` for comments.
> 
> `x86_64-unknown-unknown` can be simplified to `x86_64` (the default is ELF).
I prefer to use `x86_64-unknown-unknown`, it seems more clear to me.


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

https://reviews.llvm.org/D72225