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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

First set of purely minor stylistic comments as I start to get familiar with the code.



================
Comment at: llvm/include/llvm/MC/MCAsmBackend.h:59
+  /// Check if the target need to do instruction alignment.
+  virtual bool needAlign(MCObjectStreamer &OS) const { return false; };
 
----------------
This looks like it can be combined w/the allowAutoPadding function which I recently added.  


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:34
 #include "llvm/Support/TargetRegistry.h"
+#include "llvm/Support/raw_ostream.h"
 
----------------
Remove the reordering here.


================
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 "
----------------
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)


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:443
   // Branches only need to be aligned in 32-bit or 64-bit mode.
-  if (!(STI.getFeatureBits()[X86::Mode64Bit] ||
-        STI.getFeatureBits()[X86::Mode32Bit]))
+  if (!(STI.hasFeature(X86::Mode64Bit) || STI.hasFeature(X86::Mode32Bit)))
     return false;
----------------
This looks to be a formatting change?  If so, remove.  You can commit this without separate review if desired.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:498
+      case X86::CS:
+        return 0x2e;
+      case X86::SS:
----------------
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?)


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

https://reviews.llvm.org/D72225