[PATCH] Fix for codegen bug that could cause illegal cmn instruction generation

[Duncan P. N. Exon Smith]

I have some style nitpicks below.  The change itself looks
superficially correct to me, but you might wait to hear from
someone that knows better before committing...

On 2014 Apr 8, at 13:13, Louis Gerbarg <lgg at apple.com> wrote:

> Attached are two patches. The first adds a flag to disable the dead register definition pass in the ARM64 backend, which was helpful to me while diagnosing this issue. In the second patch.

If possible you should add a testcase for this as well, since
otherwise someone is likely to remove it later as dead code.

> The second is a patch to fix a bug caused by an interaction between the dead register definition pass and the the code that materializes frame indexed operands that could can cause illegal instructions to be generated in the case of large stack frames. Essentially what happened is that in some cases the instruction that was being used to calculate the address of the frame indexed value would need to be expanded into multiple instructions, which ends up expanding into multiple adds into the definition register.  If the register was not used for anything else then the dead definition pass had already removed and replaced that with the zero register, which means that when that expansion reused the definition as an intermediary the zero register ends up as a source for a CMN (ADDS) instruction, which is illegal. This patch tests to see if any of the operands an instruction are frame indexes.
> 
> --- a/lib/Target/ARM64/ARM64DeadRegisterDefinitionsPass.cpp
> +++ b/lib/Target/ARM64/ARM64DeadRegisterDefinitionsPass.cpp
> @@ -30,7 +30,7 @@ private:
>    const TargetRegisterInfo *TRI;
>    bool implicitlyDefinesSubReg(unsigned Reg, const MachineInstr *MI);
>    bool processMachineBasicBlock(MachineBasicBlock *MBB);
> -
> +  bool usesFrameIndex(const MachineInstr *MI);
>  public:
>    static char ID; // Pass identification, replacement for typeid.
>    explicit ARM64DeadRegisterDefinitions() : MachineFunctionPass(ID) {}
> @@ -57,47 +57,62 @@ bool ARM64DeadRegisterDefinitions::implicitlyDefinesSubReg(
>    return false;
>  }
>  
> +bool ARM64DeadRegisterDefinitions::usesFrameIndex(const MachineInstr *MI) {
> +  for (int i = 0, e = MI->getNumOperands(); i != e; ++i) {
> +    if (MI->getOperand(i).isFI())
> +      return true;
> +  }
> +  return false;
> +}

I think this would be better as a local function, as in:

    static bool usesFrameIndex(const MachineInstr *MI) {
      // ...
    }

Then the class definition change is unnecessary.

> +
>  bool
>  ARM64DeadRegisterDefinitions::processMachineBasicBlock(MachineBasicBlock *MBB) {
>    bool Changed = false;
>    for (MachineBasicBlock::iterator I = MBB->begin(), E = MBB->end(); I != E;
>         ++I) {
>      MachineInstr *MI = I;
> -    for (int i = 0, e = MI->getDesc().getNumDefs(); i != e; ++i) {
> -      MachineOperand &MO = MI->getOperand(i);
> -      if (MO.isReg() && MO.isDead() && MO.isDef()) {
> -        assert(!MO.isImplicit() && "Unexpected implicit def!");
> -        DEBUG(dbgs() << "  Dead def operand #" << i << " in:\n    ";
> -              MI->print(dbgs()));
> -        // Be careful not to change the register if it's a tied operand.
> -        if (MI->isRegTiedToUseOperand(i)) {
> -          DEBUG(dbgs() << "    Ignoring, def is tied operand.\n");
> -          continue;
> -        }
> -        // Don't change the register if there's an implicit def of a subreg.
> -        if (implicitlyDefinesSubReg(MO.getReg(), MI)) {
> -          DEBUG(dbgs() << "    Ignoring, implicitly defines subregister.\n");
> -          continue;
> -        }
> -        // Make sure the instruction take a register class that contains
> -        // the zero register and replace it if so.
> -        unsigned NewReg;
> -        switch (MI->getDesc().OpInfo[i].RegClass) {
> -        default:
> -          DEBUG(dbgs() << "    Ignoring, register is not a GPR.\n");
> -          continue;
> -        case ARM64::GPR32RegClassID:
> -          NewReg = ARM64::WZR;
> -          break;
> -        case ARM64::GPR64RegClassID:
> -          NewReg = ARM64::XZR;
> -          break;
> +    if (!usesFrameIndex(MI)) {

Invert this condition and continue early.  No need for the
extra indentation.

In particular:

    if (usesFrameIndex(MI)) {
      // We need to skip this instruction because while it appears to have a
      // dead def it uses a frame index which might expand into a multi
      // instruction sequence during EPI
      DEBUG(dbgs() << "    Ignoring, operand is frame index\n");
      continue;
    }

> +      for (int i = 0, e = MI->getDesc().getNumDefs(); i != e; ++i) {
> +        MachineOperand &MO = MI->getOperand(i);
> +        if (MO.isReg() && MO.isDead() && MO.isDef()) {
> +          assert(!MO.isImplicit() && "Unexpected implicit def!");
> +          DEBUG(dbgs() << "  Dead def operand #" << i << " in:\n    ";
> +                MI->print(dbgs()));
> +          // Be careful not to change the register if it's a tied operand.
> +          if (MI->isRegTiedToUseOperand(i)) {
> +            DEBUG(dbgs() << "    Ignoring, def is tied operand.\n");
> +            continue;
> +          }
> +          // Don't change the register if there's an implicit def of a subreg.
> +          if (implicitlyDefinesSubReg(MO.getReg(), MI)) {
> +            DEBUG(dbgs() << "    Ignoring, implicitly defines subregister.\n");
> +            continue;
> +          }
> +          // Make sure the instruction take a register class that contains
> +          // the zero register and replace it if so.
> +          unsigned NewReg;
> +          switch (MI->getDesc().OpInfo[i].RegClass) {
> +          default:
> +            DEBUG(dbgs() << "    Ignoring, register is not a GPR.\n");
> +            continue;
> +          case ARM64::GPR32RegClassID:
> +            NewReg = ARM64::WZR;
> +            break;
> +          case ARM64::GPR64RegClassID:
> +            NewReg = ARM64::XZR;
> +            break;
> +          }
> +          DEBUG(dbgs() << "    Replacing with zero register. New:\n      ");
> +          MO.setReg(NewReg);
> +          DEBUG(MI->print(dbgs()));
> +          ++NumDeadDefsReplaced;
>          }
> -        DEBUG(dbgs() << "    Replacing with zero register. New:\n      ");
> -        MO.setReg(NewReg);
> -        DEBUG(MI->print(dbgs()));
> -        ++NumDeadDefsReplaced;
>        }
> +    } else {
> +      // We need to skip this instruction because while it appears to have a
> +      // dead def it uses a frame index which might expand into a multi
> +      // instruction sequence during EPI
> +      DEBUG(dbgs() << "    Ignoring, operand is frame index\n");
>      }
>    }
>    return Changed;
> diff --git a/test/CodeGen/ARM64/dead-def-frame-index.ll b/test/CodeGen/ARM64/dead-def-frame-index.ll
> new file mode 100644
> index 0000000..876c985
> --- /dev/null
> +++ b/test/CodeGen/ARM64/dead-def-frame-index.ll
> @@ -0,0 +1,18 @@
> +; RUN: llc < %s | FileCheck %s
> +
> +target datalayout = "e-m:o-i64:64-i128:128-n32:64-S128"
> +target triple = "arm64-apple-ios7.0.0"
> +
> +; Function Attrs: nounwind ssp uwtable
> +define i32 @test1() #0 {
> +  %tmp1 = alloca i8
> +  %tmp2 = alloca i32, i32 4096
> +  %tmp3 = icmp eq i8* %tmp1, null
> +  %tmp4 = zext i1 %tmp3 to i32
> +
> +  ret i32 %tmp4
> +
> +  ; CHECK-LABEL: test1
> +  ; CHECK:   adds [[TEMP:[a-z0-9]+]], sp, #16384
> +  ; CHECK:   adds [[TEMP]], [[TEMP]], #15
> +}
> -- 
> 1.8.5.2 (Apple Git-48)
>