Addressing a very strict assert check in CostAnnotationWriter::emitInstructionAnnot

[knaumov]

Summary

The assert checks that every instruction must be annotated by this point while it is not necessary. If the inlining process was interrupted because the threshold was reached, the rest of the instructions would not be annotated which triggers the assert.
The test shows the situation in which it can happen. With the assert, the test will crash.

[mtrofin]

lgtm

llvm/lib/Analysis/InlineCost.cpp

731

Nit: remove space between return and ; (probably clang-format would handle it)

[MaskRay]

Please include in the description of the revert commit (0fa793e798701358e42b1c289b28206bde028427) why it was reverted.

llvm/test/Transforms/Inline/print-instructions-deltas-unfinished.ll

2

Add a file-level comment about the purpose of the test.

[MaskRay]

Note, your original commit 66947d05fd193bb8948943a62455d617974f2012 and several recent commits don't correctly have Differential Revision: https://reviews.llvm.org/Dxxxxx

So the differential revision was not closed automatically when you pushed the commit to origin/master.

I usually do this when committing: arcfilter; git pull --rebase origin master; last-minute-testing && git push origin HEAD:master where arcfilter is a shell function which drops unneeded Phabricator tags

arcfilter () {
  arc amend
  git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend -F -
}

[MaskRay]

@knaumov Please don't revert a patch just because the differential was not closed automatically. You can close a revision manually (click "Add Action" -> "Close Revision")

llvm/lib/Analysis/InlineCost.cpp

 
 STATISTIC(NumCallsAnalyzed, "Number of call sites analyzed");
 
 static cl::opt<int>
     DefaultThreshold("inlinedefault-threshold", cl::Hidden, cl::init(225),
                      cl::ZeroOrMore,
                      cl::desc("Default amount of inlining to perform"));
 
-static cl::opt<bool> PrintDebugInstructionDeltas("print-instruction-deltas",
-    cl::Hidden, cl::init(false),
+static cl::opt<bool> PrintDebugInstructionDeltas(
+    "print-instruction-deltas", cl::Hidden, cl::init(false),
     cl::desc("Prints deltas of cost and threshold per instruction"));
 
 static cl::opt<int> InlineThreshold(
     "inline-threshold", cl::Hidden, cl::init(225), cl::ZeroOrMore,
     cl::desc("Control the amount of inlining to perform (default = 225)"));
 
 static cl::opt<int> HintThreshold(
     "inlinehint-threshold", cl::Hidden, cl::init(325), cl::ZeroOrMore,
struct InstructionCostDetail {
 
   bool hasThresholdChanged() const { return ThresholdAfter != ThresholdBefore; }
 };
 
 class CostAnnotationWriter : public AssemblyAnnotationWriter {
 public:
   // This DenseMap stores the delta change in cost and threshold after
   // accounting for the given instruction.
-  DenseMap <const Instruction *, InstructionCostDetail> CostThresholdMap;
+  DenseMap<const Instruction *, InstructionCostDetail> CostThresholdMap;
 
   virtual void emitInstructionAnnot(const Instruction *I,
-                                        formatted_raw_ostream &OS);
 };
 
 class CallAnalyzer : public InstVisitor<CallAnalyzer, bool> {
   typedef InstVisitor<CallAnalyzer, bool> Base;
   friend class InstVisitor<CallAnalyzer, bool>;
 
 protected:
   virtual ~CallAnalyzer() {}
if (SingleBB && TI->getNumSuccessors() > 1) {
       SingleBB = false;
     }
   }
 
   void onInstructionAnalysisStart(const Instruction *I) override {
     // This function is called to store the initial cost of inlining before
     // the given instruction was assessed.
     if (!PrintDebugInstructionDeltas)
-        return ;
+      return;
     Writer.CostThresholdMap[I].CostBefore = Cost;
     Writer.CostThresholdMap[I].ThresholdBefore = Threshold;
   }
 
   void onInstructionAnalysisFinish(const Instruction *I) override {
     // This function is called to find new values of cost and threshold after
     // the instruction has been assessed.
     if (!PrintDebugInstructionDeltas)
-        return ;
+      return;
     Writer.CostThresholdMap[I].CostAfter = Cost;
     Writer.CostThresholdMap[I].ThresholdAfter = Threshold;
   }
 
   InlineResult finalizeAnalysis() override {
     // Loops generally act a lot like calls in that they act like barriers to
     // movement, require a certain amount of setup, etc. So when optimising for
     // size, we penalise any call sites that perform loops. We do this after all

 }
 
 void CallAnalyzer::disableSROAForArg(AllocaInst *SROAArg) {
   onDisableSROA(SROAArg);
   EnabledSROAAllocas.erase(SROAArg);
   disableLoadElimination();
 }
 
-void CostAnnotationWriter::emitInstructionAnnot(
-    const Instruction *I, formatted_raw_ostream &OS) {
-    // The cost of inlining of the given instruction is printed always.
-    // The threshold delta is printed only when it is non-zero. It happens
-    // when we decided to give a bonus at a particular instruction.
-    assert(CostThresholdMap.count(I) > 0 &&
-           "Expected each instruction to have an instruction annotation");
-    const auto &Record = CostThresholdMap[I];
-    OS << "; cost before = " << Record.CostBefore
-       << ", cost after = " << Record.CostAfter
-       << ", threshold before = " << Record.ThresholdBefore
-       << ", threshold after = " << Record.ThresholdAfter << ", ";
-    OS << "cost delta = " << Record.getCostDelta();
-    if (Record.hasThresholdChanged())
-      OS << ", threshold delta = " << Record.getThresholdDelta();
-    OS << "\n";
+void CostAnnotationWriter::emitInstructionAnnot(const Instruction *I,
+                                                formatted_raw_ostream &OS) {
+  if (CostThresholdMap.count(I) == 0) {
+    OS << "; No analysis for the instruction\n";
+    return;
+  }
 }
 
 /// If 'V' maps to a SROA candidate, disable SROA for it.
 void CallAnalyzer::disableSROA(Value *V) {
   if (auto *SROAArg = getSROAArgForValueOrNull(V)) {
     disableSROAForArg(SROAArg);
   }
 }
bool CallAnalyzer::isGEPFree(GetElementPtrInst &GEP) {
   SmallVector<Value *, 4> Operands;
   Operands.push_back(GEP.getOperand(0));
   for (User::op_iterator I = GEP.idx_begin(), E = GEP.idx_end(); I != E; ++I)
     if (Constant *SimpleOp = SimplifiedValues.lookup(*I))
       Operands.push_back(SimpleOp);
     else
       Operands.push_back(*I);
   return TargetTransformInfo::TCC_Free ==
-         TTI.getUserCost(&GEP, Operands, TargetTransformInfo::TCK_SizeAndLatency);
+         TTI.getUserCost(&GEP, Operands,
+                         TargetTransformInfo::TCK_SizeAndLatency);
 }
 
 bool CallAnalyzer::visitAlloca(AllocaInst &I) {
   // 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)) {

llvm/test/Transforms/Inline/print-instructions-deltas-unfinished.ll

+; Require asserts for -debug-only
+; REQUIRES: asserts
+
+; This test ensures that the hadling of instructions which were not analyzed by
+; '-print-instruction-deltas' flag due to the early exit was done correctly.
+
+; RUN: opt < %s -inline -debug-only=inline-cost -disable-output -print-instruction-deltas -inline-threshold=0 2>&1 | FileCheck %s
+
+; CHECK: No analysis for the instruction
+; CHECK:   ret void
+
+declare void @callee1()
+
+define void @bar() {
+  call void @callee1()
+  ret void
+}
+
+define void @foo() {
+  call void @bar()
+  ret void
+}