[InlineCost] Remove visitUnaryInstruction()

[aeubanks]

Summary

The simplifyInstruction() in visitUnaryInstruction() does not trigger
for all of check-llvm. Looking at all delegates to UnaryInstruction in
InstVisitor, the only instructions that either don't have a visitor in
CallAnalyzer, or redirect to UnaryInstruction, are VAArgInst and Alloca.
VAArgInst will never get simplified, and visitUnaryInstruction(Alloca)
would always return false anyway.

[aeubanks]

disableSROA

[lebedev.ri]

Not really familiar enough with this code to review, but sounds reasonable i guess.

[mtrofin]

reasonable simplification, my only concern is future evolution - like if a new IR instruction is introduced that would break the invariants you're describing; but I suppose we can cross that bridge when we get there.

[davidxl]

llvm/lib/Analysis/InlineCost.cpp

1372

Perhaps add a debug assert that this returns false always (for non-debug path, skip the simplification call for compile time).

[aeubanks]

llvm/lib/Analysis/InlineCost.cpp

1372

there isn't really a good place to put that assert, unless we want to define a CallAnalyzer::visitUnaryInstruction only in assert builds, which seems weird

[davidxl]

llvm/lib/Analysis/InlineCost.cpp

1372

The change to in visitAlloca is fine. To address the concern about future IR (mtrofin), how about keeping visitUnaryInstruction, but with a very simple body -- when it is called in the future, it will be caught and handled here.

{
assert(0);
return false;
}

[aeubanks]

llvm/lib/Analysis/InlineCost.cpp

1372

I'm not sure that it makes sense to special case the assert future instructions that are specifically unary instructions. What about future binary instructions? I'd say it makes more sense to have some assert in visitInstruction(), but I don't think there's some universal assert that can be applied to all potential future instructions.

[davidxl]

lgtm

llvm/lib/Analysis/InlineCost.cpp

1372

Fair enough.

llvm/lib/Analysis/InlineCost.cpp

protected:
   // Our visit overrides.
   bool visitAlloca(AllocaInst &I);
   bool visitPHI(PHINode &I);
   bool visitGetElementPtr(GetElementPtrInst &I);
   bool visitBitCast(BitCastInst &I);
   bool visitPtrToInt(PtrToIntInst &I);
   bool visitIntToPtr(IntToPtrInst &I);
   bool visitCastInst(CastInst &I);
-  bool visitUnaryInstruction(UnaryInstruction &I);
   bool visitCmpInst(CmpInst &I);
   bool visitSub(BinaryOperator &I);
   bool visitBinaryOperator(BinaryOperator &I);
   bool visitFNeg(UnaryOperator &I);
   bool visitLoad(LoadInst &I);
   bool visitStore(StoreInst &I);
   bool visitExtractValue(ExtractValueInst &I);
   bool visitInsertValue(InsertValueInst &I);
for (const Use &Op : GEP.indices())
     else
       Operands.push_back(Op);
   return TTI.getUserCost(&GEP, Operands,
                          TargetTransformInfo::TCK_SizeAndLatency) ==
          TargetTransformInfo::TCC_Free;
 }
 
 bool CallAnalyzer::visitAlloca(AllocaInst &I) {
+  disableSROA(I.getOperand(0));
+
   // Check whether inlining will turn a dynamic alloca into a static
   // alloca and handle that case.
   if (I.isArrayAllocation()) {
     Constant *Size = SimplifiedValues.lookup(I.getArraySize());
     if (auto *AllocSize = dyn_cast_or_null<ConstantInt>(Size)) {
       // Sometimes a dynamic alloca could be converted into a static alloca
       // after this constant prop, and become a huge static alloca on an
       // unconditional CFG path. Avoid inlining if this is going to happen above
       // a threshold.
       // FIXME: If the threshold is removed or lowered too much, we could end up
       // being too pessimistic and prevent inlining non-problematic code. This
       // could result in unintended perf regressions. A better overall strategy
       // is needed to track stack usage during inlining.
       Type *Ty = I.getAllocatedType();
       AllocatedSize = SaturatingMultiplyAdd(
           AllocSize->getLimitedValue(), DL.getTypeAllocSize(Ty).getKnownMinSize(),
           AllocatedSize);
-      if (AllocatedSize > InlineConstants::MaxSimplifiedDynamicAllocaToInline) {
+      if (AllocatedSize > InlineConstants::MaxSimplifiedDynamicAllocaToInline)
         HasDynamicAlloca = true;
-        return false;
-      }
-      return Base::visitAlloca(I);
-    }
   }
 
   // Accumulate the allocated size.
   if (I.isStaticAlloca()) {
     Type *Ty = I.getAllocatedType();
     AllocatedSize =
         SaturatingAdd(DL.getTypeAllocSize(Ty).getKnownMinSize(), AllocatedSize);
   }
 
-  // We will happily inline static alloca instructions.
-  if (I.isStaticAlloca())
-    return Base::visitAlloca(I);
-
   // FIXME: This is overly conservative. Dynamic allocas are inefficient for
   // a variety of reasons, and so we would like to not inline them into
   // functions which don't currently have a dynamic alloca. This simply
   // disables inlining altogether in the presence of a dynamic alloca.
+  if (!I.isStaticAlloca())
     HasDynamicAlloca = true;
+
   return false;
 }
 
 bool CallAnalyzer::visitPHI(PHINode &I) {
   // FIXME: We need to propagate SROA *disabling* through phi nodes, even
   // though we don't want to propagate it's bonuses. The idea is to disable
   // SROA if it *might* be used in an inappropriate manner.
 
bool CallAnalyzer::visitCastInst(CastInst &I) {
   default:
     break;
   }
 
   return TTI.getUserCost(&I, TargetTransformInfo::TCK_SizeAndLatency) ==
          TargetTransformInfo::TCC_Free;
 }
 
-bool CallAnalyzer::visitUnaryInstruction(UnaryInstruction &I) {
-  Value *Operand = I.getOperand(0);
-  if (simplifyInstruction(I, [&](SmallVectorImpl<Constant *> &COps) {
-        return ConstantFoldInstOperands(&I, COps[0], DL);
-      }))
-    return true;
-
-  // Disable any SROA on the argument to arbitrary unary instructions.
-  disableSROA(Operand);
-
-  return false;
-}
-
 bool CallAnalyzer::paramHasAttr(Argument *A, Attribute::AttrKind Attr) {
   return CandidateCall.paramHasAttr(A->getArgNo(), Attr);
 }
 
 bool CallAnalyzer::isKnownNonNullInCallee(Value *V) {
   // Does the *call site* have the NonNull attribute set on an argument?  We
   // use the attribute on the call site to memoize any analysis done in the
   // caller. This will also trip if the callee function has a non-null