Commit 44fa47c9 authored by Huihui Zhang's avatar Huihui Zhang
Browse files

[ARM][ConstantIslands] Fix stack mis-alignment caused by undoLRSpillRestore. 

Summary:
It is not safe for ARMConstantIslands to undoLRSpillRestore. PrologEpilogInserter is
the one to ensure stack alignment, taking into consideration LR is spilled or not.

For noreturn function with StackAlignment 8 (function contains call/alloc),
undoLRSpillRestore cause stack be mis-aligned. Fixing stack alignment in
ARMConstantIslands doesn't give us much benefit, as undo LR spill/restore only
occur in large function with near branches only, also doesn't have callee-saved LR spill.

Reviewers: t.p.northover, rengolin, efriedma, apazos, samparker, ostannard

Reviewed By: ostannard

Subscribers: dmgreen, ostannard, kristof.beyls, hiraditya, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D75288
parent dffbaa40

llvm/lib/Target/ARM/ARMConstantIslandPass.cpp

+0 −40
Original line number Diff line number Diff line
@@ -206,10 +206,6 @@ namespace { 
    /// T2JumpTables - Keep track of all the Thumb2 jumptable instructions.

    SmallVector<MachineInstr*, 4> T2JumpTables;



    /// HasFarJump - True if any far jump instruction has been emitted during

    /// the branch fix up pass.

    bool HasFarJump;



    MachineFunction *MF;

    MachineConstantPool *MCP;

    const ARMBaseInstrInfo *TII;
@@ -270,7 +266,6 @@ namespace { 
    bool fixupImmediateBr(ImmBranch &Br);

    bool fixupConditionalBr(ImmBranch &Br);

    bool fixupUnconditionalBr(ImmBranch &Br);

    bool undoLRSpillRestore();

    bool optimizeThumb2Instructions();

    bool optimizeThumb2Branches();

    bool reorderThumb2JumpTables();
@@ -363,7 +358,6 @@ bool ARMConstantIslands::runOnMachineFunction(MachineFunction &mf) { 
  isThumb1 = AFI->isThumb1OnlyFunction();

  isThumb2 = AFI->isThumb2Function();



  HasFarJump = false;

  bool GenerateTBB = isThumb2 || (isThumb1 && SynthesizeThumb1TBB);



  // Renumber all of the machine basic blocks in the function, guaranteeing that
@@ -456,11 +450,6 @@ bool ARMConstantIslands::runOnMachineFunction(MachineFunction &mf) { 
  // After a while, this might be made debug-only, but it is not expensive.

  verify();



  // If LR has been forced spilled and no far jump (i.e. BL) has been issued,

  // undo the spill / restore of LR if possible.

  if (isThumb && !HasFarJump && AFI->isLRSpilledForFarJump())

    MadeChange |= undoLRSpillRestore();



  // Save the mapping between original and cloned constpool entries.

  for (unsigned i = 0, e = CPEntries.size(); i != e; ++i) {

    for (unsigned j = 0, je = CPEntries[i].size(); j != je; ++j) {
@@ -1633,7 +1622,6 @@ ARMConstantIslands::fixupUnconditionalBr(ImmBranch &Br) { 
  BBInfoVector &BBInfo = BBUtils->getBBInfo();

  BBInfo[MBB->getNumber()].Size += 2;

  BBUtils->adjustBBOffsetsAfter(MBB);

  HasFarJump = true;

  ++NumUBrFixed;



  LLVM_DEBUG(dbgs() << "  Changed B to long jump " << *MI);
@@ -1735,34 +1723,6 @@ ARMConstantIslands::fixupConditionalBr(ImmBranch &Br) { 
  return true;

}



/// undoLRSpillRestore - Remove Thumb push / pop instructions that only spills

/// LR / restores LR to pc. FIXME: This is done here because it's only possible

/// to do this if tBfar is not used.

bool ARMConstantIslands::undoLRSpillRestore() {

  bool MadeChange = false;

  for (unsigned i = 0, e = PushPopMIs.size(); i != e; ++i) {

    MachineInstr *MI = PushPopMIs[i];

    // First two operands are predicates.

    if (MI->getOpcode() == ARM::tPOP_RET &&

        MI->getOperand(2).getReg() == ARM::PC &&

        MI->getNumExplicitOperands() == 3) {

      // Create the new insn and copy the predicate from the old.

      BuildMI(MI->getParent(), MI->getDebugLoc(), TII->get(ARM::tBX_RET))

          .add(MI->getOperand(0))

          .add(MI->getOperand(1));

      MI->eraseFromParent();

      MadeChange = true;

    } else if (MI->getOpcode() == ARM::tPUSH &&

               MI->getOperand(2).getReg() == ARM::LR &&

               MI->getNumExplicitOperands() == 3) {

      // Just remove the push.

      MI->eraseFromParent();

      MadeChange = true;

    }

  }

  return MadeChange;

}



bool ARMConstantIslands::optimizeThumb2Instructions() {

  bool MadeChange = false;

llvm/lib/Target/ARM/ARMFrameLowering.cpp

+2 −5
Original line number Diff line number Diff line
@@ -1768,8 +1768,7 @@ void ARMFrameLowering::determineCalleeSaves(MachineFunction &MF, 
  if (!LRSpilled && AFI->isThumb1OnlyFunction()) {

    unsigned FnSize = EstimateFunctionSizeInBytes(MF, TII);

    // Force LR to be spilled if the Thumb function size is > 2048. This enables

    // use of BL to implement far jump. If it turns out that it's not needed

    // then the branch fix up path will undo it.

    // use of BL to implement far jump.

    if (FnSize >= (1 << 11)) {

      CanEliminateFrame = false;

      ForceLRSpill = true;
@@ -2120,10 +2119,8 @@ void ARMFrameLowering::determineCalleeSaves(MachineFunction &MF, 
    }

  }



  if (ForceLRSpill) {

  if (ForceLRSpill)

    SavedRegs.set(ARM::LR);

    AFI->setLRIsSpilledForFarJump(true);

  }

  AFI->setLRIsSpilled(SavedRegs.test(ARM::LR));

}

llvm/lib/Target/ARM/ARMMachineFunctionInfo.h

+0 −7
Original line number Diff line number Diff line
@@ -58,10 +58,6 @@ class ARMFunctionInfo : public MachineFunctionInfo { 
  /// emitPrologue.

  bool RestoreSPFromFP = false;



  /// LRSpilledForFarJump - True if the LR register has been for spilled to

  /// enable far jump.

  bool LRSpilledForFarJump = false;



  /// LRSpilled - True if the LR register has been for spilled for

  /// any reason, so it's legal to emit an ARM::tBfar (i.e. "bl").

  bool LRSpilled = false;
@@ -162,9 +158,6 @@ public: 
  bool isLRSpilled() const { return LRSpilled; }

  void setLRIsSpilled(bool s) { LRSpilled = s; }



  bool isLRSpilledForFarJump() const { return LRSpilledForFarJump; }

  void setLRIsSpilledForFarJump(bool s) { LRSpilledForFarJump = s; }



  unsigned getFramePtrSpillOffset() const { return FramePtrSpillOffset; }

  void setFramePtrSpillOffset(unsigned o) { FramePtrSpillOffset = o; }

llvm/test/CodeGen/Thumb/remove-unneeded-push-pop.ll

deleted100644 → 0
+0 −1052

File deleted.

Preview size limit exceeded, changes collapsed.

llvm/test/CodeGen/Thumb/stack-mis-alignment.ll

0 → 100644
+18 −0
Original line number Diff line number Diff line 
; RUN: llc -O0 < %s | FileCheck %s



; For noreturn function with StackAlignment 8 (function contains call/alloc),

; check that lr is saved to keep the stack aligned.

; CHECK: push    {lr}



target datalayout = "e-m:e-p:32:32-Fi8-i64:64-v128:64:128-a:0:32-n32-S64"

target triple = "thumbv5e-none-linux-gnueabi"



define dso_local i32 @f() noreturn nounwind {

entry:

  call i32 @llvm.arm.space(i32 2048, i32 undef)

  tail call i32 @exit(i32 0)

  unreachable

}



declare i32 @llvm.arm.space(i32, i32)

declare dso_local i32 @exit(i32)