[PATCH] D70157: Align branches within 32-Byte boundary

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

Overall comment: this whole change needs more comments, everywhere. Both for the added functions, and for the test cases. There is almost no description of what's happening, and it could really use it.



================
Comment at: llvm/lib/MC/MCAssembler.cpp:1041
+
+void MCAssembler::moveSymbol(const MCFragment *Src, MCFragment *Dst) const {
+  if (!(Src && Dst && Dst->getKind() == MCFragment::FT_MachineDependent))
----------------
I think this is broken -- moving symbols around like this risks causing evaluations of symbol offsets which may have already happened to be wrong. 

Also it's ugly, and I can't tell why it's necessary, because there's no comments.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:418
+
+bool X86AsmBackend::isReturn(const MCInst &MI) const {
+  unsigned Opcode = MI.getOpcode();
----------------
This set of functions down to isIndirectBranch() seems unnecessary. Pushing one line
  const MCInstrDesc &InstDesc = MCII.get(Inst.getOpcode());
into needAlign(const MCInst &Inst), and then just using InstDesc.isReturn() etc. would be fine.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:463
+  const X86::SecondMFInstKind BranchKind = classifySecond(Jcc, MCII);
+  return X86::isMacroFused(CmpKind,BranchKind);
+  llvm_unreachable("unknown fusion type");
----------------
please run something like "git clang-format HEAD~1" to re-format your patch.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:467
+
+char X86AsmBackend::choosePrefixValue(const MCInst &MI) const {
+  for (const auto &Operand : MI) {
----------------
Comment on why this is doing what it's doing?


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:532
+
+bool X86AsmBackend::needAlignBranch() const {
+  return AlignBoundarySize != 0 &&
----------------
Confusing name, and doesn't need to be a separate function than needAlign(const MCAssembler &, MCSection *). Just merge it into that.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:537
+
+bool X86AsmBackend::needAlignJcc() const {
+  return AlignBoundarySize != 0 &&
----------------
These functions are only used in one place, and it doesn't make it more readable to split them off. Just merge them into needAlign(const MCInst &Inst) or alignBranchesBegin as appropriate.

But also -- AlignBoundarySize shouldn't even need to be checked here, since it's already be checked in needAlign(const MCAssembler &, MCSection *), which is always called first.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:567
+
+bool X86AsmBackend::needAlign(const MCAssembler &Assembler,
+                              MCSection *Sec) const {
----------------
Doesn't need to have the same name as the next needAlign, clearer if these two overloads are given different names.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:731-732
+                                    char Prefix) const {
+  if (!(Prefix == 0x2e || Prefix == 0x36 || Prefix == 0x3e || Prefix == 0x26 ||
+        Prefix == 0x64 || Prefix == 0x65))
+    return false;
----------------
Why?


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h:106
+  // The classification for the first instruction in macro fusion.
+  enum class FirstMFInstKind {
+    // TEST
----------------
"MF" is not a readable abbreviation here -- spell out "MacroFusion" instead. It was OK locally in the "X86MacroFusion.cpp" file, as the filename gave you a hint, but not here, where you have no such hint.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h:134
+  /// macro-fusion.
+  inline FirstMFInstKind classifyFirstOpcode(unsigned Opcode) {
+    switch (Opcode) {
----------------
This function is also named poorly, after being moved to this more-generic location. "Classify" doesn't tell me anything about this being related to macro-fusion.


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

https://reviews.llvm.org/D70157