[PATCH] D75438: [X86] Reduce the number of emitted fragments due to branch align

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/MC/MCAssembler.cpp:1022
   BF.setSize(NewSize);
-  Layout.invalidateFragmentsFrom(&BF);
   return true;
----------------
Do me a favour and leave this line in for this patch.  It can be removed in a follow up, but the assumptions about offset calculation caching are subtle and it's clearly conservatively correct to invalidate too much.  :)


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:454
+
+  // We need to ensure that further data aren't added to the current data
+  // fragment.
----------------
skan wrote:
> reames wrote:
> > I believe you can simply insert a new empty data fragment instead of adding the special state to all data fragments.  
> > 
> > Also, you only need to do so if the last fragment was a data fragment.  
> If the next intruction is emitted into a relaxable fragment, the empty data fragment will be unused, is it acceptable to you?
Yes.  This is an idiom we already use in a few places and we can fix them all at once if it becomes an issue.  


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:434
+  if (PendingBoundaryAlign &&
+      OS.getCurrentFragment()->getPrevNode() == PendingBoundaryAlign)
     // Macro fusion actually happens and there is no other fragment inserted
----------------
Does this need to be a runtime check?  Or can it be an assert?

"assert(OS.getCurrentFragment()->getPrevNode() == PendingBoundaryAlign)" to be clear.

Also, add braces.  Even though there's a single statement the block comment deserves clear demarcation.  


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:439
+    // Do nothing here since we already inserted a BoudaryAlign fragment when
+    // we met the first instruction in the fused pair.
+    //
----------------
Add to comment: and we'll tie them together in branchBoundaryEnd


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:454
 
-  PrevInst = Inst;
+  else if (needAlignInst(Inst) || ((AlignBranchType & X86::AlignBranchFused) &&
+                                   isFirstMacroFusibleInst(Inst, *MCII)))
----------------
Drop the else since the previous if returned.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:472
+
+  if (PendingBoundaryAlign) {
+    PendingBoundaryAlign->setLastFragment(OS.getCurrentFragment());
----------------
Might be good to add an assert here about the expected instructions between pending BA and this one.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:477
+
+  // We need to ensure that further data isn't added to the current data
+  // fragment. The easiest way is to insert a new empty DataFragment.
----------------
I believe we only need this bit of code if we tied the aligned instruction into a previous BA.  (i.e. we can early return above if there's no pending BA.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75438