[PATCH] D39840: [MC][X86] Code padding for performance stability - Branch instructions and targets alignment

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:80
   X86AsmBackend(const Target &T, StringRef CPU)
-      : MCAsmBackend(), CPU(CPU),
-        MaxNopLength((CPU == "slm") ? 7 : 15) {
+      : MCAsmBackend(std::move(
+            std::unique_ptr<MCCodePadder>(new X86::X86MCCodePadder(CPU)))),
----------------
The std::move here should be unnecessary the unique_ptr would be a temporary.

Maybe better to just do MCAsmBackend(llvm::make_unique<X86::X86MCCodePadder>(CPU))


================
Comment at: lib/Target/X86/MCTargetDesc/X86MCCodePadder.cpp:38
+      CPU != "core-avx-i" && CPU != "haswell" && CPU != "core-avx2" &&
+      CPU != "broadwell" && CPU != "skylake")
+    return;
----------------
Should skylake-avx512 be in this list? What about "cannonlake". This seems destined to become unmaintainable quickly. Is there some way we can put this in a subtarget property.


================
Comment at: lib/Target/X86/MCTargetDesc/X86MCCodePadder.cpp:69
+  return
+      // Immidiate jmps
+      opcode == JAE_1 || opcode == JAE_2 || opcode == JAE_4 || opcode == JA_1 ||
----------------
Immediate*


================
Comment at: lib/Target/X86/MCTargetDesc/X86MCCodePadder.cpp:70
+      // Immidiate jmps
+      opcode == JAE_1 || opcode == JAE_2 || opcode == JAE_4 || opcode == JA_1 ||
+      opcode == JA_2 || opcode == JA_4 || opcode == JBE_1 || opcode == JBE_2 ||
----------------
zvi wrote:
> Is it possible to encode this info in the .td's with something like isConditionalBranch=1?
Inst.getDesc().isConditionalBranch() might work?


================
Comment at: lib/Target/X86/MCTargetDesc/X86MCCodePadder.cpp:87
+static bool isFarOrIndirectUncoditionalJump(const MCInst &Inst) {
+  unsigned int opcode = Inst.getOpcode();
+  return
----------------
Variable names should be capitalized.

LLVM normally prefers "unsigned" over "unsigned int"


================
Comment at: lib/Target/X86/MCTargetDesc/X86MCCodePadder.cpp:99
+  unsigned int opcode = Inst.getOpcode();
+  return isFarOrIndirectUncoditionalJump(Inst) || opcode == JMP_1 ||
+         opcode == JMP_2 || opcode == JMP_4;
----------------
Unconditional*


Repository:
  rL LLVM

https://reviews.llvm.org/D39840