[GlobalISel] Fix legalizer trying to process a deleted instruction

[aemerson]

Summary

[GlobalISel] Fix legalizer trying to process a deleted instruction.

In some cases an instruction is deleted from the block during combining, however it can still exist in the legalizer worklist.

This change modifies the combiner routines to add the given MI to the dead instruction list rather than trying to remove it from the block themselves. The responsibility is then on the caller to delete the instruction from the block and any worklists.

This bug was exposed by D38621 on a test suite build.

[aemerson]

Forgot to add llvm-commits, pasting description again:

[GlobalISel] Fix legalizer trying to process a deleted instruction.

In some cases an instruction is deleted from the block during combining, however it can still exist in the legalizer worklist.

This change modifies the combiner routines to add the given MI to the dead instruction list rather than trying to remove it from the block themselves. The responsibility is then on the caller to delete the instruction from the block and any worklists.

This bug was exposed by D38621 on a test suite build.

[qcolombet]

LGTM with one comment: could we have the whole pattern being a helper function?

[aemerson]

Thanks, I've put the idiom into a helper and committed. Let me know if it's not what you had in mind.

Cheers,
Amara

llvm/trunk/include/llvm/CodeGen/GlobalISel/LegalizerCombiner.h

bool tryCombineAnyExt(MachineInstr &MI,
     MachineInstr *DefMI = MRI.getVRegDef(MI.getOperand(1).getReg());
     if (DefMI->getOpcode() == TargetOpcode::G_TRUNC) {
       DEBUG(dbgs() << ".. Combine MI: " << MI;);
       unsigned DstReg = MI.getOperand(0).getReg();
       unsigned SrcReg = DefMI->getOperand(1).getReg();
       Builder.setInstr(MI);
       // We get a copy/trunc/extend depending on the sizes
       Builder.buildAnyExtOrTrunc(DstReg, SrcReg);
-      MI.eraseFromParent();
-      if (MRI.use_empty(DefMI->getOperand(0).getReg()))
-        DeadInsts.push_back(DefMI);
+      markInstAndDefDead(MI, *DefMI, DeadInsts);
       return true;
     }
     return false;
   }
 
   bool tryCombineZExt(MachineInstr &MI,
                       SmallVectorImpl<MachineInstr *> &DeadInsts) {
 
     if (MI.getOpcode() != TargetOpcode::G_ZEXT)
       return false;
     MachineInstr *DefMI = MRI.getVRegDef(MI.getOperand(1).getReg());
     if (DefMI->getOpcode() == TargetOpcode::G_TRUNC) {
       DEBUG(dbgs() << ".. Combine MI: " << MI;);
       Builder.setInstr(MI);
       unsigned DstReg = MI.getOperand(0).getReg();
       unsigned ZExtSrc = MI.getOperand(1).getReg();
       LLT ZExtSrcTy = MRI.getType(ZExtSrc);
       LLT DstTy = MRI.getType(DstReg);
       APInt Mask = APInt::getAllOnesValue(ZExtSrcTy.getSizeInBits());
       auto MaskCstMIB = Builder.buildConstant(DstTy, Mask.getZExtValue());
       unsigned TruncSrc = DefMI->getOperand(1).getReg();
       // We get a copy/trunc/extend depending on the sizes
       auto SrcCopyOrTrunc = Builder.buildAnyExtOrTrunc(DstTy, TruncSrc);
       Builder.buildAnd(DstReg, SrcCopyOrTrunc, MaskCstMIB);
-      MI.eraseFromParent();
-      if (MRI.use_empty(DefMI->getOperand(0).getReg()))
-        DeadInsts.push_back(DefMI);
+      markInstAndDefDead(MI, *DefMI, DeadInsts);
       return true;
     }
     return false;
   }
 
   bool tryCombineSExt(MachineInstr &MI,
                       SmallVectorImpl<MachineInstr *> &DeadInsts) {
 
if (DefMI->getOpcode() == TargetOpcode::G_TRUNC) {
       unsigned SizeDiff = DstTy.getSizeInBits() - SExtSrcTy.getSizeInBits();
       auto SizeDiffMIB = Builder.buildConstant(DstTy, SizeDiff);
       unsigned TruncSrcReg = DefMI->getOperand(1).getReg();
       // We get a copy/trunc/extend depending on the sizes
       auto SrcCopyExtOrTrunc = Builder.buildAnyExtOrTrunc(DstTy, TruncSrcReg);
       auto ShlMIB = Builder.buildInstr(TargetOpcode::G_SHL, DstTy,
                                        SrcCopyExtOrTrunc, SizeDiffMIB);
       Builder.buildInstr(TargetOpcode::G_ASHR, DstReg, ShlMIB, SizeDiffMIB);
-      MI.eraseFromParent();
-      if (MRI.use_empty(DefMI->getOperand(0).getReg()))
-        DeadInsts.push_back(DefMI);
+      markInstAndDefDead(MI, *DefMI, DeadInsts);
       return true;
     }
     return false;
   }
 
   bool tryCombineMerges(MachineInstr &MI,
                         SmallVectorImpl<MachineInstr *> &DeadInsts) {
 
if (NumMergeRegs < NumDefs) {
           MRI.getType(MergeI->getOperand(1).getReg()))
         return false;
 
       for (unsigned Idx = 0; Idx < NumDefs; ++Idx)
         MRI.replaceRegWith(MI.getOperand(Idx).getReg(),
                            MergeI->getOperand(Idx + 1).getReg());
     }
 
-    MI.eraseFromParent();
-    if (MRI.use_empty(MergeI->getOperand(0).getReg()))
-      DeadInsts.push_back(MergeI);
+    markInstAndDefDead(MI, *MergeI, DeadInsts);
     return true;
   }
 
   /// Try to combine away MI.
   /// Returns true if it combined away the MI.
-  /// Caller should not rely in MI existing as it may be deleted.
   /// Adds instructions that are dead as a result of the combine
-  // into DeadInsts
+  /// into DeadInsts, which can include MI.
   bool tryCombineInstruction(MachineInstr &MI,
                              SmallVectorImpl<MachineInstr *> &DeadInsts) {
     switch (MI.getOpcode()) {
     default:
       return false;
     case TargetOpcode::G_ANYEXT:
       return tryCombineAnyExt(MI, DeadInsts);
     case TargetOpcode::G_ZEXT:
       return tryCombineZExt(MI, DeadInsts);
     case TargetOpcode::G_SEXT:
       return tryCombineSExt(MI, DeadInsts);
     case TargetOpcode::G_UNMERGE_VALUES:
       return tryCombineMerges(MI, DeadInsts);
     }
   }
+
+private:
+  /// Mark MI as dead. If a def of one of MI's operands, DefMI, would also be
+  /// dead due to MI being killed, then mark DefMI as dead too.
+  void markInstAndDefDead(MachineInstr &MI, MachineInstr &DefMI,
+                          SmallVectorImpl<MachineInstr *> &DeadInsts) {
+    DeadInsts.push_back(&MI);
+    if (MRI.hasOneUse(DefMI.getOperand(0).getReg()))
+      DeadInsts.push_back(&DefMI);
+  }
 };
 
 } // namespace llvm

llvm/trunk/test/CodeGen/AArch64/GlobalISel/combine-anyext-crash.mir

+# RUN: llc -O0 -run-pass=legalizer -global-isel %s -o - | FileCheck %s
+--- |
+  target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+  target triple = "aarch64--"
+  
+  define void @test_anyext_crash() {
+  entry:
+    br label %block2
+  
+  block2:
+    %0 = trunc i16 0 to i8
+    %1 = uitofp i8 %0 to double
+    br label %block2
+  }
+  
+
+...
+---
+name:            test_anyext_crash
+alignment:       2
+legalized:       false
+registers:       
+  - { id: 0, class: _, preferred-register: '' }
+  - { id: 1, class: _, preferred-register: '' }
+  - { id: 2, class: _, preferred-register: '' }
+body:             |
+  bb.1:
+   ; Check we don't crash due to trying to legalize a dead instruction.
+   ; CHECK-LABEL: test_anyext_crash
+   ; CHECK-LABEL: bb.1:
+    successors: %bb.2
+  
+    %0(s16) = G_CONSTANT i16 0
+  
+  bb.2:
+    successors: %bb.2
+  
+    %1(s8) = G_TRUNC %0(s16)
+    %2(s64) = G_UITOFP %1(s8)
+    G_BR %bb.2
+
+...