[ArgPromotion] Delay dead GEP removal until doPromotion.

[fhahn]

Summary

Currently ArgPromotion removes dead GEPs as part of the legality check
in isSafeToPromoteArgument. If no promotion happens, this means the pass
claims no modifications happened, even though GEPs were removed.

This patch fixes the issue by delaying removal of dead GEPs until
doPromotion: isSafeToPromoteArgument can simply skips dead GEPs and
the code in doPromotion dealing with GEPs is updated to account for
dead GEPs. Once we committed to promotion, it should be safe to
remove dead GEPs.

Alternatively isSafeToPromoteArgument could return an additional boolean
to indicate whether it made changes, but this is quite cumbersome and
there should be no real benefit of weeding out some dead GEPs here if we
do not perform promotion.

I added a test for the case where dead GEPs need to be removed when
promotion happens in 578c5a0c6e71.

Fixes PR47477.

[fhahn]

This has also been reported as https://bugs.llvm.org/show_bug.cgi?id=47477

[jdoerfert]

LGTM

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp

if (ByValArgsToTransform.count(&*I)) {
       ++NumArgumentsDead;
     } else {
       // Okay, this is being promoted. This means that the only uses are loads
       // or GEPs which are only used by loads
 
       // In this table, we will track which indices are loaded from the argument
       // (where direct loads are tracked as no indices).
       ScalarizeTable &ArgIndices = ScalarizedElements[&*I];
-      for (User *U : I->users()) {
-        Instruction *UI = cast<Instruction>(U);
+      for (auto Iter = I->user_begin(), End = I->user_end(); Iter != End;) {
+        Instruction *UI = cast<Instruction>(*Iter++);
         Type *SrcTy;
         if (LoadInst *L = dyn_cast<LoadInst>(UI))
           SrcTy = L->getType();
         else
           SrcTy = cast<GetElementPtrInst>(UI)->getSourceElementType();
+        // Skip dead GEPs and remove them.
+        if (isa<GetElementPtrInst>(UI) && UI->use_empty()) {
+          UI->eraseFromParent();
+          continue;
+        }
+
         IndicesVector Indices;
         Indices.reserve(UI->getNumOperands() - 1);
         // Since loads will only have a single operand, and GEPs only a single
         // non-index operand, this will record direct loads without any indices,
         // and gep+loads with the GEP indices.
         for (User::op_iterator II = UI->op_begin() + 1, IE = UI->op_end();
              II != IE; ++II)
           Indices.push_back(cast<ConstantInt>(*II)->getSExtValue());
while (!I->use_empty()) {
                "Load element should sort to front!");
         I2->setName(I->getName() + ".val");
         LI->replaceAllUsesWith(&*I2);
         LI->eraseFromParent();
         LLVM_DEBUG(dbgs() << "*** Promoted load of argument '" << I->getName()
                           << "' in function '" << F->getName() << "'\n");
       } else {
         GetElementPtrInst *GEP = cast<GetElementPtrInst>(I->user_back());
+        assert(!GEP->use_empty() &&
+               "GEPs without uses should be cleaned up already");
         IndicesVector Operands;
         Operands.reserve(GEP->getNumIndices());
         for (User::op_iterator II = GEP->idx_begin(), IE = GEP->idx_end();
              II != IE; ++II)
           Operands.push_back(cast<ConstantInt>(*II)->getSExtValue());
 
         // GEPs with a single 0 index can be merged with direct loads
         if (Operands.size() == 1 && Operands.front() == 0)
if (LoadInst *LI = dyn_cast<LoadInst>(UR)) {
       Operands.push_back(0);
 
       if (!UpdateBaseTy(LI->getType()))
         return false;
     } else if (GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(UR)) {
       if (GEP->use_empty()) {
         // Dead GEP's cause trouble later.  Just remove them if we run into
         // them.
-        GEP->eraseFromParent();
-        // TODO: This runs the above loop over and over again for dead GEPs
-        // Couldn't we just do increment the UI iterator earlier and erase the
-        // use?
-        return isSafeToPromoteArgument(Arg, ByValTy, AAR, MaxElements);
+        continue;
       }
 
       if (!UpdateBaseTy(GEP->getSourceElementType()))
         return false;
 
       // Ensure that all of the indices are constants.
       for (User::op_iterator i = GEP->idx_begin(), e = GEP->idx_end(); i != e;
            ++i)

llvm/test/Transforms/ArgumentPromotion/dead-gep-no-promotion.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -argpromotion -S %s | FileCheck %s
+
+@glob = external global i32*
+
+; No arguments in @callee can be promoted, but it contains a dead GEP. Make
+; sure it is not removed, as we do not perform any promotion.
+define i32 @caller(i32* %ptr) {
+; CHECK-LABEL: @caller(
+; CHECK-NEXT:    call void @callee(i32* [[PTR:%.*]], i32* [[PTR]], i32* [[PTR]])
+; CHECK-NEXT:    ret i32 0
+;
+  call void @callee(i32* %ptr, i32* %ptr, i32* %ptr)
+  ret i32 0
+}
+
+define internal void @callee(i32* %arg, i32* %arg1, i32* %arg2) {
+; CHECK-LABEL: define internal void @callee(
+; CHECK-NEXT:    call void @external_fn(i32* [[ARG:%.*]], i32* [[ARG1:%.*]])
+; CHECK-NEXT:    [[DEAD_GEP:%.*]] = getelementptr inbounds i32, i32* [[ARG1]], i32 17
+; CHECK-NEXT:    store i32* [[ARG2:%.*]], i32** @glob, align 8
+; CHECK-NEXT:    ret void
+;
+  call void @external_fn(i32* %arg, i32* %arg1)
+  %dead.gep = getelementptr inbounds i32, i32* %arg1, i32 17
+  store i32* %arg2, i32** @glob, align 8
+  ret  void
+}
+
+declare void @external_fn(i32*, i32*)