NFC: Migrate CodeMetrics to work on InstructionCost

[sdesmalen]

Summary

This patch migrates cost values and arithmetic to work on InstructionCost.
When the interfaces to TargetTransformInfo are changed, any InstructionCost
state will propagate naturally.

See this patch for the introduction of the type: https://reviews.llvm.org/D91174
See this thread for context: http://lists.llvm.org/pipermail/llvm-dev/2020-November/146408.html

[sdesmalen]

llvm/lib/Analysis/CodeMetrics.cpp

204

FYI, the reason for dereferencing the Optional cost value returned by getValue() unconditionally is that if the number of instructions for a basic-block cannot be costed, the cost-model is broken, which should never happen.

[david-arm]

llvm/lib/Analysis/CodeMetrics.cpp

118

Hi @sdesmalen, I'm not entirely sure why we're using an InstructionCost here to be honest, when NumInsts is an unsigned and any new costs are immediately dereferenced below at line 183 after calling TTI.getUserCost?

183

Isn't this a rolling value that needs incrementing? It's an unsigned value in CodeMetrics.h.

204

I think actually NumInstsThisBB is guaranteed to be valid because NumInstsBeforeThisBB is initialised with an unsigned, and NumInstsProxy must be valid because if it wasn't we'd have already crashed with an assert on line 183.

[sdesmalen]

llvm/lib/Analysis/CodeMetrics.cpp

118

Part of the value of InstructionCost comes when we'll add saturation, so that the value doesn't wrap for 'magical' values returned by TTI.get*Cost functions (targets may return some very high value to indicate something is expensive). By accumulating the values from TTI.get*Cost into an InstructionCost, it already prepares these cases for the future. I tried to imply that a bit with "using InstructionCost's arithmetic properties".

[david-arm]

llvm/lib/Analysis/CodeMetrics.cpp

183

Sorry ignore that comment. :) I missed the fact that it's picking up the incremented NumInstsProxy.

[david-arm]

LGTM!

llvm/lib/Analysis/CodeMetrics.cpp

116–123

nit: Could you specifically mention saturation here as an example? Not sure about others, but it certainly helped me to understand the change more when I considered the saturation.

[sdesmalen]

Updated comment.

[david-arm]

LGTM! Thanks for updating the comments!

llvm/lib/Analysis/CodeMetrics.cpp

 #include "llvm/Analysis/CodeMetrics.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/Analysis/AssumptionCache.h"
 #include "llvm/Analysis/LoopInfo.h"
 #include "llvm/Analysis/TargetTransformInfo.h"
 #include "llvm/Analysis/ValueTracking.h"
 #include "llvm/IR/Function.h"
 #include "llvm/Support/Debug.h"
+#include "llvm/Support/InstructionCost.h"
 
 #define DEBUG_TYPE "code-metrics"
 
 using namespace llvm;
 
 static void
 appendSpeculatableOperands(const Value *V,
                            SmallPtrSetImpl<const Value *> &Visited,

 }
 
 /// Fill in the current structure with information gleaned from the specified
 /// block.
 void CodeMetrics::analyzeBasicBlock(
     const BasicBlock *BB, const TargetTransformInfo &TTI,
     const SmallPtrSetImpl<const Value *> &EphValues, bool PrepareForLTO) {
   ++NumBlocks;
-  unsigned NumInstsBeforeThisBB = NumInsts;
+  // Use a proxy variable for NumInsts of type InstructionCost, so that it can
+  // use InstructionCost's arithmetic properties such as saturation when this
+  // feature is added to InstructionCost.
+  // When storing the value back to NumInsts, we can assume all costs are Valid
+  // because the IR should not contain any nodes that cannot be costed. If that
+  // happens the cost-model is broken.
+  InstructionCost NumInstsProxy = NumInsts;
+  InstructionCost NumInstsBeforeThisBB = NumInsts;
   for (const Instruction &I : *BB) {
     // Skip ephemeral values.
     if (EphValues.count(&I))
       continue;
 
     // Special handling for calls.
     if (const auto *Call = dyn_cast<CallBase>(&I)) {
       if (const Function *F = Call->getCalledFunction()) {
if (const CallInst *CI = dyn_cast<CallInst>(&I)) {
       if (CI->isConvergent())
         convergent = true;
     }
 
     if (const InvokeInst *InvI = dyn_cast<InvokeInst>(&I))
       if (InvI->cannotDuplicate())
         notDuplicatable = true;
 
-    NumInsts += TTI.getUserCost(&I, TargetTransformInfo::TCK_CodeSize);
+    NumInstsProxy += TTI.getUserCost(&I, TargetTransformInfo::TCK_CodeSize);
+    NumInsts = *NumInstsProxy.getValue();
   }
 
   if (isa<ReturnInst>(BB->getTerminator()))
     ++NumRets;
 
   // We never want to inline functions that contain an indirectbr.  This is
   // incorrect because all the blockaddress's (in static global initializers
   // for example) would be referring to the original function, and this indirect
   // jump would jump from the inlined copy of the function into the original
   // function which is extremely undefined behavior.
   // FIXME: This logic isn't really right; we can safely inline functions
   // with indirectbr's as long as no other function or global references the
   // blockaddress of a block within the current function.  And as a QOI issue,
   // if someone is using a blockaddress without an indirectbr, and that
   // reference somehow ends up in another function or global, we probably
   // don't want to inline this function.
   notDuplicatable |= isa<IndirectBrInst>(BB->getTerminator());
 
   // Remember NumInsts for this BB.
-  NumBBInsts[BB] = NumInsts - NumInstsBeforeThisBB;
+  InstructionCost NumInstsThisBB = NumInstsProxy - NumInstsBeforeThisBB;
+  NumBBInsts[BB] = *NumInstsThisBB.getValue();
 }