[PATCH] D101817: [MC][X86] Automatic alignment for Macro-Op Fusion

[Amir Ayupov via Phabricator via llvm-commits]

Amir added inline comments.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:165-172
+    if (X86AlignForMacroFusion) {
+      assert(!X86AlignBranchWithin32BBoundaries &&
+             "X86AlignForMacroFusion is incompatible with "
+             "X86AlignBranchWithin32BBoundaries");
+      AlignBoundary = assumeAligned(64);
+      AlignBranchType.addKind(X86::AlignBranchFused);
+      AlignBranchType.addKind(X86::AlignBranchJcc);
----------------
skan wrote:
> Amir wrote:
> > skan wrote:
> > > In fact, X86AlignForMacroFusion is compatible with X86AlignBranchWithin32BBoundaries, you can set AlignBoundary to 64 and add  AlignBranchFused to AlignBranchType if both of the options exist.
> > No, they're semantically incompatible: X86AlignBranchWithin32BBoundaries would prevent macrofusion pair from crossing the 32B boundary (stronger alignment restriction), while X86AlignForMacroFusion would prevent macrofusion pair being perfectly split by 64B boundary.
> > So if X86AlignForMacroFusion is on, it's known not to satisfy X86AlignBranchWithin32BBoundaries restrictions. 
> > (Conversely, if X86AlignBranchWithin32BBoundaries is on, X86AlignForMacroFusion is satisfied automatically).
> > 
> > They're further incompatible by the fact that X86AlignForMacroFusion flag enables the new BoundaryAlign behavior `isAvoidEndAlign` which performs the same alignment as NeverAlign.
> "So if X86AlignForMacroFusion is on, it's known not to satisfy X86AlignBranchWithin32BBoundaries restrictions.
> (Conversely, if X86AlignBranchWithin32BBoundaries is on, X86AlignForMacroFusion is satisfied automatically)."
> 
> The requirements of X86AlignForMacroFusion and X86AlignBranchWithin32BBoundaries can be met at the same time, so they are compatible.
> 
> `isAvoidEndAlign`  is a detail about implementation and is not a good proof for the incompatibility.
> 
> Meanwhile, AlignBranchType, AlignBoundary can be also be set by options X86AlignBranchBoundary, X86AlignBranch. So the assert here is not correct.
I see what you mean. We can handle X86AlignForMacroFusion first and then override it if X86AlignBranchWithin32BBoundaries is set as it's a stronger alignment requirement. Let me fix that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101817