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

[Omer Paparo Bivas via Phabricator via llvm-commits]

opaparo marked 9 inline comments as done.
opaparo added inline comments.


================
Comment at: lib/Target/X86/MCTargetDesc/X86MCCodePadder.cpp:38
+      CPU != "core-avx-i" && CPU != "haswell" && CPU != "core-avx2" &&
+      CPU != "broadwell" && CPU != "skylake")
+    return;
----------------
gadi.haber wrote:
> craig.topper wrote:
> > 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.
> isn't it safer just to check that the CPU is atom instead?
> The rule is good for any future CPU by default.
I rather have an inclusive list than an exclusive list.
That way you have more control and no CPU can "snick in".

I don't see a better way to achieve that. The Backend has no access to a MCSubtargetInfo (And so X86AsmBackend's constructor also checks for individual CPUs when initalizing the field HasNopl, for example).
Do you know of a way around it, or a better solution?


================
Comment at: lib/Target/X86/MCTargetDesc/X86MCCodePadder.h:23
+
+/// The X86-specific class incharge of all code padding decisions for the X86
+/// target.
----------------
gadi.haber wrote:
> zvi wrote:
> > *in charge
> decisions --> policies ?
"policies" is more of an implementation detail


================
Comment at: lib/Target/X86/MCTargetDesc/X86MCCodePadder.h:107
+  ///
+  /// \returns true iff \p Inst is a branch (all types of jmps and calls).
+  bool instructionRequiresPaddingFragment(const MCInst &Inst) const override;
----------------
gadi.haber wrote:
> both direct and indirect?
Yes.


https://reviews.llvm.org/D39840