fast-isel: remove dead code after falling back from selecting call instructions (PR20863)

[hans]

Summary

Previously, fast-isel would not clean up after trying but failing to select a call instruction, because it would have called flushLocalValueMap() which moves the insertion point, making SavedInsertPt in selectInstruction() invalid. We can fix this by making SavedInsertPt a member variable, and having flushLocalValueMap() update it.

This removes some redundant code at-O0, and more importantly fixes PR20863. Please take a look!

[echristo]

I think this looks reasonable.

[ributzka]

LGTM

Could you also add an unit test that checks that the local value area didn't get accidentally deleted as dead code too?

[hans]

Could you also add an unit test that checks that the local value area didn't get accidentally deleted as dead code too?

Like this? (See attached patch)

[ributzka]

Yup

[hans]

Closed by commit rL217401 (authored by @hans).

[rnk]

Nice, that was easy. :)

include/llvm/CodeGen/FastISel.h

private:
   /// instructions.
   unsigned materializeRegForValue(const Value *V, MVT VT);
 
   /// \brief Clears LocalValueMap and moves the area for the new local variables
   /// to the beginning of the block. It helps to avoid spilling cached variables
   /// across heavy instructions like calls.
   void flushLocalValueMap();
 
+  /// \brief Insertion point before trying to select the current instruction.
+  MachineBasicBlock::iterator SavedInsertPt;
+
   /// \brief Add a stackmap or patchpoint intrinsic call's live variable
   /// operands to a stackmap or patchpoint machine instruction.
   bool addStackMapLiveVars(SmallVectorImpl<MachineOperand> &Ops,
                            const CallInst *CI, unsigned StartIdx);
   bool lowerCallOperands(const CallInst *CI, unsigned ArgIdx, unsigned NumArgs,
                          const Value *Callee, bool ForceRetVoidTy,
                          CallLoweringInfo &CLI);
 };
 
 } // end namespace llvm
 
 #endif

lib/CodeGen/SelectionDAG/FastISel.cpp

bool FastISel::lowerArguments() {
   }
   return true;
 }
 
 void FastISel::flushLocalValueMap() {
   LocalValueMap.clear();
   LastLocalValue = EmitStartPt;
   recomputeInsertPt();
+  SavedInsertPt = FuncInfo.InsertPt;
 }
 
 bool FastISel::hasTrivialKill(const Value *V) {
   // Don't consider constants or arguments to have trivial kills.
   const Instruction *I = dyn_cast<Instruction>(V);
   if (!I)
     return false;
 
bool FastISel::selectInstruction(const Instruction *I) {
   // Just before the terminator instruction, insert instructions to
   // feed PHI nodes in successor blocks.
   if (isa<TerminatorInst>(I))
     if (!handlePHINodesInSuccessorBlocks(I->getParent()))
       return false;
 
   DbgLoc = I->getDebugLoc();
 
-  MachineBasicBlock::iterator SavedInsertPt = FuncInfo.InsertPt;
+  SavedInsertPt = FuncInfo.InsertPt;
 
   if (const auto *Call = dyn_cast<CallInst>(I)) {
     const Function *F = Call->getCalledFunction();
     LibFunc::Func Func;
 
     // As a special case, don't handle calls to builtin library functions that
     // may be translated directly to target instructions.
     if (F && !F->hasLocalLinkage() && F->hasName() &&
bool FastISel::selectInstruction(const Instruction *I) {
 
   // First, try doing target-independent selection.
   if (!SkipTargetIndependentISel) {
     if (selectOperator(I, I->getOpcode())) {
       ++NumFastIselSuccessIndependent;
       DbgLoc = DebugLoc();
       return true;
     }
-    // Remove dead code.  However, ignore call instructions since we've flushed
-    // the local value map and recomputed the insert point.
-    if (!isa<CallInst>(I)) {
-      recomputeInsertPt();
-      if (SavedInsertPt != FuncInfo.InsertPt)
-        removeDeadCode(FuncInfo.InsertPt, SavedInsertPt);
-    }
+    // Remove dead code.
     SavedInsertPt = FuncInfo.InsertPt;
   }
   // Next, try calling the target to attempt to handle the instruction.
   if (fastSelectInstruction(I)) {
     ++NumFastIselSuccessTarget;
     DbgLoc = DebugLoc();
     return true;
   }
-  // Remove dead code.  However, ignore call instructions since we've flushed
-  // the local value map and recomputed the insert point.
-  if (!isa<CallInst>(I)) {
-    recomputeInsertPt();
-    if (SavedInsertPt != FuncInfo.InsertPt)
-      removeDeadCode(FuncInfo.InsertPt, SavedInsertPt);
-  }
+  // Remove dead code.
 
   DbgLoc = DebugLoc();
   // Undo phi node updates, because they will be added again by SelectionDAG.
   if (isa<TerminatorInst>(I))
     FuncInfo.PHINodesToUpdate.resize(FuncInfo.OrigNumPHINodesToUpdate);
   return false;
 }
 

test/CodeGen/X86/fast-isel-x86.ll

entry:
   ret void
 ; CHECK-LABEL: test4:
 ; CHECK: subl $28
 ; CHECK: leal (%esp), %ecx
 ; CHECK: calll _test4fastccsret
 ; CHECK: addl $28
 }
 declare fastcc void @test4fastccsret(%struct.a* sret)
+
+
+; Check that fast-isel cleans up when it fails to lower a call instruction.
+define void @test5() {
+entry:
+  %call = call i32 @test5dllimport(i32 42)
+  ret void
+; CHECK-LABEL: test5:
+; CHECK-NOT: movl $42, (%esp)
+; CHECK: movl %esp, [[REGISTER:%[a-z]+]]
+; CHECK: movl $42, ([[REGISTER]])
+; CHECK: movl __imp__test5dllimport
+}
+declare dllimport i32 @test5dllimport(i32)