CodeGen: Remove MaxBytesForAlignment from MachineBasicBlock

[arsenm]

Summary

This was an overcomplicated way of tracking a constant used in the
AsmPrinter. This was not serialized in MIR. None of the in tree code
was changing this dynamically based on individual blocks. It was
always set to a subtarget defined constant.

This should probably be moved out of TargetLowering and into
MCAsmInfo. The only place I think this should change codegen is
BranchRelaxation which didn't account for potentially ignored
alignment in the first place.

[arsenm]

ping

[dmgreen]

I'm not sure I understand. Why do you think that MaxBytesForAlignment isn't a per-basic-block parameter? Any aligned block can have a MaxBytesForAlignment, and they could all be different from one block to the next, just as alignments can change.

[arsenm]

In D124092#3550117, @dmgreen wrote:

I'm not sure I understand. Why do you think that MaxBytesForAlignment isn't a per-basic-block parameter? Any aligned block can have a MaxBytesForAlignment, and they could all be different from one block to the next, just as alignments can change.

But that isn't actually used. This is only ever sourced from a target constant value. This is an unused feature essentially

[courbet]

In D124092#3550117, @dmgreen wrote:

I'm not sure I understand. Why do you think that MaxBytesForAlignment isn't a per-basic-block parameter? Any aligned block can have a MaxBytesForAlignment, and they could all be different from one block to the next, just as alignments can change.

I think the point is that it's never actually used anywhere.

[dmgreen]

It's used by AArch64, even if it's just a single value at the moment. getMaxPermittedBytesForAlignment should probably be called something like getMaxPermittedBytesForLoopAlignment. GCC sets different alignment/maxbytes for Loops vs Jumptable (vs Functions). You could imagine having different alignments/maxbytes for outer loops vs inner loops, or based on some other characteristics of the loop.

It is a value that seems to fit most naturally paired together with the Alignment on the MachineBasicBlock.

[arsenm]

In D124092#3550457, @dmgreen wrote:

It's used by AArch64, even if it's just a single value at the moment. getMaxPermittedBytesForAlignment should probably be called something like getMaxPermittedBytesForLoopAlignment. GCC sets different alignment/maxbytes for Loops vs Jumptable (vs Functions). You could imagine having different alignments/maxbytes for outer loops vs inner loops, or based on some other characteristics of the loop.

You could imagine it, but unless someone implements something using this, it's bitrotting, incomplete infrastructure.

[arsenm]

It's been over a year, and this still isn't serialized or set from anywhere. It needs to be dropped or fully implemented

[dmgreen]

@chill was changing how some of this works in https://reviews.llvm.org/D156235. What are the other parts that are needed? In MachineBasicBlock::print and MIParser::parseBasicBlockDefinition it needs to print and parse like it does for align?

llvm/include/llvm/CodeGen/MachineBasicBlock.h

public:
   const uint32_t *getEndClobberMask(const TargetRegisterInfo *TRI) const;
 
   /// Return alignment of the basic block.
   Align getAlignment() const { return Alignment; }
 
   /// Set alignment of the basic block.
   void setAlignment(Align A) { Alignment = A; }
 
-  void setAlignment(Align A, unsigned MaxBytes) {
-    setAlignment(A);
-    setMaxBytesForAlignment(MaxBytes);
-  }
-
-  /// Return the maximum amount of padding allowed for aligning the basic block.
-  unsigned getMaxBytesForAlignment() const { return MaxBytesForAlignment; }
-
-  /// Set the maximum amount of padding allowed for aligning the basic block
-  void setMaxBytesForAlignment(unsigned MaxBytes) {
-    MaxBytesForAlignment = MaxBytes;
-  }
-
   /// Returns true if the block is a landing pad. That is this basic block is
   /// entered via an exception handler.
   bool isEHPad() const { return IsEHPad; }
 
   /// Indicates the block is a landing pad.  That is this basic block is entered
   /// via an exception handler.
   void setIsEHPad(bool V = true) { IsEHPad = V; }
 

llvm/include/llvm/CodeGen/TargetLowering.h

#include "llvm/IR/ConstrainedOps.def"
 
   /// Return the preferred function alignment.
   Align getPrefFunctionAlignment() const { return PrefFunctionAlignment; }
 
   /// Return the preferred loop alignment.
   virtual Align getPrefLoopAlignment(MachineLoop *ML = nullptr) const;
 
   /// Return the maximum amount of bytes allowed to be emitted when padding for
-  /// alignment
-  virtual unsigned
-  getMaxPermittedBytesForAlignment(MachineBasicBlock *MBB) const;
+  /// alignment of a MachineBasicBlock.
+  unsigned getMaxPermittedBytesForAlignment() const {
+    return MaxBytesForAlignment;
+  }
 
   /// Should loops be aligned even when the function is marked OptSize (but not
   /// MinSize).
   virtual bool alignLoopsWithOptSize() const { return false; }
 
   /// If the target has a standard location for the stack protector guard,
   /// returns the address of that location. Otherwise, returns nullptr.
   /// DEPRECATED: please override useLoadStackGuardNode and customize

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

 const char CodeViewLineTablesGroupDescription[] = "CodeView Line Tables";
 const char PPTimerName[] = "emit";
 const char PPTimerDescription[] = "Pseudo Probe Emission";
 const char PPGroupName[] = "pseudo probe";
 const char PPGroupDescription[] = "Pseudo Probe Emission";
 
 STATISTIC(EmittedInsts, "Number of machine instrs printed");
 
+static cl::opt<unsigned> MaxBytesForAlignmentOverride(
+    "max-bytes-for-alignment",
+    cl::desc("Forces the maximum bytes allowed to be emitted when padding for "
+             "alignment"),
+    cl::init(0), cl::Hidden);
+
 char AsmPrinter::ID = 0;
 
 namespace {
 class AddrLabelMapCallbackPtr final : CallbackVH {
   AddrLabelMap *Map = nullptr;
 
 public:
   AddrLabelMapCallbackPtr() = default;
if (MBB.isBeginSection() && !MBB.isEntryBlock()) {
     OutStreamer->switchSection(
         getObjFileLowering().getSectionForMachineBasicBlock(MF->getFunction(),
                                                             MBB, TM));
     CurrentSectionBeginSym = MBB.getSymbol();
   }
 
   // Emit an alignment directive for this block, if needed.
   const Align Alignment = MBB.getAlignment();
-  if (Alignment != Align(1))
-    emitAlignment(Alignment, nullptr, MBB.getMaxBytesForAlignment());
+  if (Alignment != Align(1)) {
+    const TargetLowering *TLI = MF->getSubtarget().getTargetLowering();
+    unsigned MaxBytes = MaxBytesForAlignmentOverride.getNumOccurrences() > 0 ?
+      MaxBytesForAlignmentOverride :
+      TLI->getMaxPermittedBytesForAlignment();
+
+    emitAlignment(Alignment, nullptr, MaxBytes);
+  }
 
   // If the block has its address taken, emit any labels that were used to
   // reference the block.  It is possible that there is more than one label
   // here, because multiple LLVM BB's may have been RAUW'd to this block after
   // the references were generated.
   if (MBB.isIRBlockAddressTaken()) {
     if (isVerbose())
       OutStreamer->AddComment("Block address taken");

llvm/lib/CodeGen/BranchRelaxation.cpp

struct BasicBlockInfo {
     unsigned Size = 0;
 
     BasicBlockInfo() = default;
 
     /// Compute the offset immediately following this block. \p MBB is the next
     /// block.
     unsigned postOffset(const MachineBasicBlock &MBB) const {
       const unsigned PO = Offset + Size;
+
+      // FIXME: Alignment is ignored if the padding is greater than
+      // getMaxPermittedBytesForAlignment
       const Align Alignment = MBB.getAlignment();
       const Align ParentAlign = MBB.getParent()->getAlignment();
       if (Alignment <= ParentAlign)
         return alignTo(PO, Alignment);
 
       // The alignment of this MBB is larger than the function's alignment, so we
       // can't tell whether or not it will insert nops. Assume that it will.
       return alignTo(PO, Alignment) + Alignment.value() - ParentAlign.value();

llvm/lib/CodeGen/MachineBlockPlacement.cpp

 
 static cl::opt<unsigned> AlignAllNonFallThruBlocks(
     "align-all-nofallthru-blocks",
     cl::desc("Force the alignment of all blocks that have no fall-through "
              "predecessors (i.e. don't add nops that are executed). In log2 "
              "format (e.g 4 means align on 16B boundaries)."),
     cl::init(0), cl::Hidden);
 
-static cl::opt<unsigned> MaxBytesForAlignmentOverride(
-    "max-bytes-for-alignment",
-    cl::desc("Forces the maximum bytes allowed to be emitted when padding for "
-             "alignment"),
-    cl::init(0), cl::Hidden);
-
 // FIXME: Find a good default for this flag and remove the flag.
 static cl::opt<unsigned> ExitBlockBias(
     "block-placement-exit-block-bias",
     cl::desc("Block frequency percentage a loop exit block needs "
              "over the original exit to be considered the new exit."),
     cl::init(0), cl::Hidden);
 
 // Definition:
if (llvm::shouldOptimizeForSize(ChainBB, PSI, MBFI.get()) &&
         !TLI->alignLoopsWithOptSize())
       continue;
 
     // Check for the existence of a non-layout predecessor which would benefit
     // from aligning this block.
     MachineBasicBlock *LayoutPred =
         &*std::prev(MachineFunction::iterator(ChainBB));
 
-    auto DetermineMaxAlignmentPadding = [&]() {
-      // Set the maximum bytes allowed to be emitted for alignment.
-      unsigned MaxBytes;
-      if (MaxBytesForAlignmentOverride.getNumOccurrences() > 0)
-        MaxBytes = MaxBytesForAlignmentOverride;
-      else
-        MaxBytes = TLI->getMaxPermittedBytesForAlignment(ChainBB);
-      ChainBB->setMaxBytesForAlignment(MaxBytes);
-    };
-
     // Force alignment if all the predecessors are jumps. We already checked
     // that the block isn't cold above.
     if (!LayoutPred->isSuccessor(ChainBB)) {
       ChainBB->setAlignment(Align);
-      DetermineMaxAlignmentPadding();
       continue;
     }
 
     // Align this block if the layout predecessor's edge into this block is
     // cold relative to the block. When this is true, other predecessors make up
     // all of the hot entries into the block and thus alignment is likely to be
     // important.
     BranchProbability LayoutProb =
         MBPI->getEdgeProbability(LayoutPred, ChainBB);
     BlockFrequency LayoutEdgeFreq = MBFI->getBlockFreq(LayoutPred) * LayoutProb;
     if (LayoutEdgeFreq <= (Freq * ColdProb)) {
       ChainBB->setAlignment(Align);
-      DetermineMaxAlignmentPadding();
     }
   }
 }
 
 /// Tail duplicate \p BB into (some) predecessors if profitable, repeating if
 /// it was duplicated into its chain predecessor and removed.
 /// \p BB    - Basic block that may be duplicated.
 ///
bool MachineBlockPlacement::runOnMachineFunction(MachineFunction &MF) {
 
   optimizeBranches();
   alignBlocks();
 
   BlockToChain.clear();
   ComputedEdges.clear();
   ChainAllocator.DestroyAll();
 
-  bool HasMaxBytesOverride =
-      MaxBytesForAlignmentOverride.getNumOccurrences() > 0;
-
-  if (AlignAllBlock)
+  if (AlignAllBlock) {
     // Align all of the blocks in the function to a specific alignment.
-    for (MachineBasicBlock &MBB : MF) {
-      if (HasMaxBytesOverride)
-        MBB.setAlignment(Align(1ULL << AlignAllBlock),
-                         MaxBytesForAlignmentOverride);
-      else
-        MBB.setAlignment(Align(1ULL << AlignAllBlock));
-    }
-  else if (AlignAllNonFallThruBlocks) {
+    for (MachineBasicBlock &MBB : MF)
+  } else if (AlignAllNonFallThruBlocks) {
     // Align all of the blocks that have no fall-through predecessors to a
     // specific alignment.
     for (auto MBI = std::next(MF.begin()), MBE = MF.end(); MBI != MBE; ++MBI) {
       auto LayoutPred = std::prev(MBI);
-      if (!LayoutPred->isSuccessor(&*MBI)) {
-        if (HasMaxBytesOverride)
-          MBI->setAlignment(Align(1ULL << AlignAllNonFallThruBlocks),
-                            MaxBytesForAlignmentOverride);
-        else
-          MBI->setAlignment(Align(1ULL << AlignAllNonFallThruBlocks));
-      }
-    }
-  }
+      if (!LayoutPred->isSuccessor(&*MBI))
   if (ViewBlockLayoutWithBFI != GVDT_None &&
       (ViewBlockFreqFuncName.empty() ||
        F->getFunction().getName().equals(ViewBlockFreqFuncName))) {
     if (RenumberBlocksBeforeView)
       MF.RenumberBlocks();
     MBFI->view("MBP." + MF.getName(), false);
   }
 

llvm/lib/CodeGen/TargetLoweringBase.cpp

 }
 
 Align TargetLoweringBase::getPrefLoopAlignment(MachineLoop *ML) const {
   if (TM.Options.LoopAlignment)
     return Align(TM.Options.LoopAlignment);
   return PrefLoopAlignment;
 }
 
-unsigned TargetLoweringBase::getMaxPermittedBytesForAlignment(
-    MachineBasicBlock *MBB) const {
-  return MaxBytesForAlignment;
-}
-
 //===----------------------------------------------------------------------===//
 //  Reciprocal Estimates
 //===----------------------------------------------------------------------===//
 
 /// Get the reciprocal estimate attribute string for a function that will
 /// override the target defaults.
 static StringRef getRecipEstimateForFunc(MachineFunction &MF) {
   const Function &F = MF.getFunction();

llvm/tools/llvm-reduce/ReducerWorkItem.cpp

if (SrcMBB.isMachineBlockAddressTaken())
       DstMBB->setMachineBlockAddressTaken();
 
     // FIXME: This is not serialized
     if (SrcMBB.hasLabelMustBeEmitted())
       DstMBB->setLabelMustBeEmitted();
 
     DstMBB->setAlignment(SrcMBB.getAlignment());
 
-    // FIXME: This is not serialized
-    DstMBB->setMaxBytesForAlignment(SrcMBB.getMaxBytesForAlignment());
-
     DstMBB->setIsEHPad(SrcMBB.isEHPad());
     DstMBB->setIsEHScopeEntry(SrcMBB.isEHScopeEntry());
     DstMBB->setIsEHCatchretTarget(SrcMBB.isEHCatchretTarget());
     DstMBB->setIsEHFuncletEntry(SrcMBB.isEHFuncletEntry());
 
     // FIXME: These are not serialized
     DstMBB->setIsCleanupFuncletEntry(SrcMBB.isCleanupFuncletEntry());
     DstMBB->setIsBeginSection(SrcMBB.isBeginSection());