[NFC] Change return type for 'shouldOptimizeForSize'

[ebrevnov]

Summary

Currently 'shouldOptimizeForSize' returns bool to indicate if function should be optimized for size or not. When 'false' is returned it could mean two things. Either we don't need to optimize for size or there is no information. For upcoming changes in LoopVectorizer I need to distinguish between these two cases.

[ebrevnov]

Nope.

[ebrevnov]

Nope2.

[tellenbach]

Hi,

this looks good beside some minor inline comments.

Could you also add a test for the case of no profile being available? shouldOptimizeForSize should then be tested for None instead of false.

Thanks!

llvm/include/llvm/CodeGen/MachineSizeOpts.h

27

Some word ist missing here: "if there is no info ? the profile"

33

Same here: Missing word in comment.

llvm/include/llvm/Transforms/Utils/SizeOpts.h

39

Please clang-format the whole signature. I think this would move BFIT *BFI) { one line up.

78

Same as above: Missing word in comment.

83

Same as above: Missing word in comment.

86

Please clang-format the whole signature.

[ebrevnov]

It turned out that I should change an approach for D67805 and this change is not needed anymore. Thanks for your time anyway. Abandoning.

llvm/include/llvm/CodeGen/MachineSizeOpts.h

 namespace llvm {
 
 class ProfileSummaryInfo;
 class MachineBasicBlock;
 class MachineBlockFrequencyInfo;
 class MachineFunction;
 
 /// Returns true if machine function \p MF is suggested to be size-optimized
-/// base on the profile.
-bool shouldOptimizeForSize(const MachineFunction *MF, ProfileSummaryInfo *PSI,
+/// base on the profile, false if not. Returns None if there is no info the
+/// profile.
+Optional<bool> shouldOptimizeForSize(const MachineFunction *MF,
+                                     ProfileSummaryInfo *PSI,
                                      const MachineBlockFrequencyInfo *BFI);
 /// Returns true if machine basic block \p MBB is suggested to be size-optimized
-/// base on the profile.
-bool shouldOptimizeForSize(const MachineBasicBlock *MBB,
+/// base on the profile, false if not. Returns None if there is no info the
+/// profile.
+Optional<bool> shouldOptimizeForSize(const MachineBasicBlock *MBB,
                                      ProfileSummaryInfo *PSI,
                                      const MachineBlockFrequencyInfo *MBFI);
 
 } // end namespace llvm
 
 #endif // LLVM_CODEGEN_MACHINE_SIZEOPTS_H

llvm/include/llvm/Transforms/Utils/SizeOpts.h

 
 namespace llvm {
 
 class BasicBlock;
 class BlockFrequencyInfo;
 class Function;
 class ProfileSummaryInfo;
 
-template<typename AdapterT, typename FuncT, typename BFIT>
-bool shouldFuncOptimizeForSizeImpl(const FuncT *F, ProfileSummaryInfo *PSI,
+template <typename AdapterT, typename FuncT, typename BFIT>
+Optional<bool> shouldFuncOptimizeForSizeImpl(const FuncT *F,
+                                             ProfileSummaryInfo *PSI,
                                              BFIT *BFI) {
   assert(F);
   if (!PSI || !BFI || !PSI->hasProfileSummary())
-    return false;
+    return None;
   if (ForcePGSO)
     return true;
   if (!EnablePGSO)
-    return false;
+    return None;
   if (PGSOColdCodeOnly ||
       (PGSOLargeWorkingSetSizeOnly && !PSI->hasLargeWorkingSetSize())) {
     // Even if the working set size isn't large, size-optimize cold code.
     return AdapterT::isFunctionColdInCallGraph(F, PSI, *BFI);
   }
   return !AdapterT::isFunctionHotInCallGraphNthPercentile(
       PSI->hasSampleProfile() ? PgsoCutoffSampleProf : PgsoCutoffInstrProf,
       F, PSI, *BFI);
 }
 
-template<typename AdapterT, typename BlockT, typename BFIT>
-bool shouldOptimizeForSizeImpl(const BlockT *BB, ProfileSummaryInfo *PSI,
-                               BFIT *BFI) {
+template <typename AdapterT, typename BlockT, typename BFIT>
+Optional<bool> shouldOptimizeForSizeImpl(const BlockT *BB,
+                                         ProfileSummaryInfo *PSI, BFIT *BFI) {
   assert(BB);
   if (!PSI || !BFI || !PSI->hasProfileSummary())
-    return false;
+    return None;
   if (ForcePGSO)
     return true;
   if (!EnablePGSO)
-    return false;
+    return None;
   if (PGSOColdCodeOnly ||
       (PGSOLargeWorkingSetSizeOnly && !PSI->hasLargeWorkingSetSize())) {
     // Even if the working set size isn't large, size-optimize cold code.
     return AdapterT::isColdBlock(BB, PSI, BFI);
   }
   return !AdapterT::isHotBlockNthPercentile(
       PSI->hasSampleProfile() ? PgsoCutoffSampleProf : PgsoCutoffInstrProf,
       BB, PSI, BFI);
 }
 
 /// Returns true if function \p F is suggested to be size-optimized base on the
-/// profile.
-bool shouldOptimizeForSize(const Function *F, ProfileSummaryInfo *PSI,
+/// profile, false if not. Returns None if there is no info the profile.
+Optional<bool> shouldOptimizeForSize(const Function *F, ProfileSummaryInfo *PSI,
                                      BlockFrequencyInfo *BFI);
 
 /// Returns true if basic block \p BB is suggested to be size-optimized base
-/// on the profile.
-bool shouldOptimizeForSize(const BasicBlock *BB, ProfileSummaryInfo *PSI,
+/// on the profile, false if not. Returns None if there is no info the profile.
+Optional<bool> shouldOptimizeForSize(const BasicBlock *BB,
+                                     ProfileSummaryInfo *PSI,
                                      BlockFrequencyInfo *BFI);
-
 } // end namespace llvm
 
 #endif // LLVM_TRANSFORMS_UTILS_SIZEOPTS_H

llvm/lib/CodeGen/MachineSizeOpts.cpp

static bool isHotBlockNthPercentile(int CutOff,
                                       ProfileSummaryInfo *PSI,
                                       const MachineBlockFrequencyInfo *MBFI) {
     return machine_size_opts_detail::isHotBlockNthPercentile(
         CutOff, MBB, PSI, MBFI);
   }
 };
 } // end anonymous namespace
 
-bool llvm::shouldOptimizeForSize(const MachineFunction *MF,
-                                 ProfileSummaryInfo *PSI,
-                                 const MachineBlockFrequencyInfo *MBFI) {
-  return shouldFuncOptimizeForSizeImpl<MachineBasicBlockBFIAdapter>(
-      MF, PSI, MBFI);
+Optional<bool>
+llvm::shouldOptimizeForSize(const MachineFunction *MF, ProfileSummaryInfo *PSI,
+  return shouldFuncOptimizeForSizeImpl<MachineBasicBlockBFIAdapter>(MF, PSI,
+                                                                    MBFI);
 }
 
-bool llvm::shouldOptimizeForSize(const MachineBasicBlock *MBB,
-                                 ProfileSummaryInfo *PSI,
-                                 const MachineBlockFrequencyInfo *MBFI) {
-  return shouldOptimizeForSizeImpl<MachineBasicBlockBFIAdapter>(
-      MBB, PSI, MBFI);
+Optional<bool>
+llvm::shouldOptimizeForSize(const MachineBasicBlock *MBB,
+  return shouldOptimizeForSizeImpl<MachineBasicBlockBFIAdapter>(MBB, PSI, MBFI);
 }

llvm/lib/CodeGen/TargetLoweringBase.cpp

bool TargetLoweringBase::isSuitableForJumpTable(const SwitchInst *SI,
                                                 BlockFrequencyInfo *BFI) const {
   // FIXME: This function check the maximum table size and density, but the
   // minimum size is not checked. It would be nice if the minimum size is
   // also combined within this function. Currently, the minimum size check is
   // performed in findJumpTable() in SelectionDAGBuiler and
   // getEstimatedNumberOfCaseClusters() in BasicTTIImpl.
   const bool OptForSize =
       SI->getParent()->getParent()->hasOptSize() ||
-      llvm::shouldOptimizeForSize(SI->getParent(), PSI, BFI);
+      llvm::shouldOptimizeForSize(SI->getParent(), PSI, BFI).getValueOr(false);
   const unsigned MinDensity = getMinimumJumpTableDensity(OptForSize);
   const unsigned MaxJumpTableSize = getMaximumJumpTableSize();
 
   // Check whether the number of cases is small enough and
   // the range is dense enough for a jump table.
   return (OptForSize || Range <= MaxJumpTableSize) &&
          (NumCases * 100 >= Range * MinDensity);
 }

llvm/lib/Transforms/Scalar/ConstantHoisting.cpp

 // which does not do all the offset calculations.
 unsigned
 ConstantHoistingPass::maximizeConstantsInRange(ConstCandVecType::iterator S,
                                            ConstCandVecType::iterator E,
                                            ConstCandVecType::iterator &MaxCostItr) {
   unsigned NumUses = 0;
 
   bool OptForSize = Entry->getParent()->hasOptSize() ||
-                    llvm::shouldOptimizeForSize(Entry->getParent(), PSI, BFI);
-  if (!OptForSize || std::distance(S,E) > 100) {
+                    llvm::shouldOptimizeForSize(Entry->getParent(), PSI, BFI)
+                        .getValueOr(false);
+  if (!OptForSize || std::distance(S, E) > 100) {
     for (auto ConstCand = S; ConstCand != E; ++ConstCand) {
       NumUses += ConstCand->Uses.size();
       if (ConstCand->CumulativeCost > MaxCostItr->CumulativeCost)
         MaxCostItr = ConstCand;
     }
     return NumUses;
   }
 

llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp

if (!Checks.empty() || !LAI.getPSE().getUnionPredicate().isAlwaysTrue()) {
       if (LAI.hasConvergentOp()) {
         LLVM_DEBUG(dbgs() << "Versioning is needed but not allowed with "
                              "convergent calls\n");
         return false;
       }
 
       auto *HeaderBB = L->getHeader();
       auto *F = HeaderBB->getParent();
-      bool OptForSize = F->hasOptSize() ||
-                        llvm::shouldOptimizeForSize(HeaderBB, PSI, BFI);
+      bool OptForSize =
+          F->hasOptSize() ||
+          llvm::shouldOptimizeForSize(HeaderBB, PSI, BFI).getValueOr(false);
       if (OptForSize) {
         LLVM_DEBUG(
             dbgs() << "Versioning is needed but not allowed when optimizing "
                       "for size.\n");
         return false;
       }
 
       if (!L->isLoopSimplifyForm()) {

llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp

TargetTransformInfo::UnrollingPreferences llvm::gatherUnrollingPreferences(
   UP.UnrollAndJam = false;
   UP.PeelProfiledIterations = true;
   UP.UnrollAndJamInnerLoopThreshold = 60;
 
   // Override with any target specific settings
   TTI.getUnrollingPreferences(L, SE, UP);
 
   // Apply size attributes
-  bool OptForSize = L->getHeader()->getParent()->hasOptSize() ||
-                    llvm::shouldOptimizeForSize(L->getHeader(), PSI, BFI);
+  bool OptForSize =
+      L->getHeader()->getParent()->hasOptSize() ||
+      llvm::shouldOptimizeForSize(L->getHeader(), PSI, BFI).getValueOr(false);
   if (OptForSize) {
     UP.Threshold = UP.OptSizeThreshold;
     UP.PartialThreshold = UP.PartialOptSizeThreshold;
     UP.MaxPercentThresholdBoost = 100;
   }
 
   // Apply any user values specified by cl::opt
   if (UnrollThreshold.getNumOccurrences() > 0)

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

Value *LibCallSimplifier::optimizeFWrite(CallInst *CI, IRBuilder<> &B) {
   return nullptr;
 }
 
 Value *LibCallSimplifier::optimizeFPuts(CallInst *CI, IRBuilder<> &B) {
   optimizeErrorReporting(CI, B, 1);
 
   // Don't rewrite fputs to fwrite when optimising for size because fwrite
   // requires more arguments and thus extra MOVs are required.
-  bool OptForSize = CI->getFunction()->hasOptSize() ||
-                    llvm::shouldOptimizeForSize(CI->getParent(), PSI, BFI);
+  bool OptForSize =
+      CI->getFunction()->hasOptSize() ||
+      llvm::shouldOptimizeForSize(CI->getParent(), PSI, BFI).getValueOr(false);
   if (OptForSize)
     return nullptr;
 
   // Check if has any use
   if (!CI->use_empty()) {
     if (isLocallyOpenedFile(CI->getArgOperand(1), CI, B, TLI))
       return emitFPutSUnlocked(CI->getArgOperand(0), CI->getArgOperand(1), B,
                                TLI);

llvm/lib/Transforms/Utils/SizeOpts.cpp

static bool isHotBlockNthPercentile(int CutOff,
                                       const BasicBlock *BB,
                                       ProfileSummaryInfo *PSI,
                                       BlockFrequencyInfo *BFI) {
     return PSI->isHotBlockNthPercentile(CutOff, BB, BFI);
   }
 };
 } // end anonymous namespace
 
-bool llvm::shouldOptimizeForSize(const Function *F, ProfileSummaryInfo *PSI,
+Optional<bool> llvm::shouldOptimizeForSize(const Function *F,
+                                           ProfileSummaryInfo *PSI,
                                            BlockFrequencyInfo *BFI) {
   return shouldFuncOptimizeForSizeImpl<BasicBlockBFIAdapter>(F, PSI, BFI);
 }
 
-bool llvm::shouldOptimizeForSize(const BasicBlock *BB, ProfileSummaryInfo *PSI,
+Optional<bool> llvm::shouldOptimizeForSize(const BasicBlock *BB,
+                                           ProfileSummaryInfo *PSI,
                                            BlockFrequencyInfo *BFI) {
   return shouldOptimizeForSizeImpl<BasicBlockBFIAdapter>(BB, PSI, BFI);
 }

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

 static ScalarEpilogueLowering
 getScalarEpilogueLowering(Function *F, Loop *L, LoopVectorizeHints &Hints,
                           ProfileSummaryInfo *PSI, BlockFrequencyInfo *BFI,
                           TargetTransformInfo *TTI, TargetLibraryInfo *TLI,
                           AssumptionCache *AC, LoopInfo *LI,
                           ScalarEvolution *SE, DominatorTree *DT,
                           const LoopAccessInfo *LAI) {
   ScalarEpilogueLowering SEL = CM_ScalarEpilogueAllowed;
+  Optional<bool> IsColdByProfile =
+      llvm::shouldOptimizeForSize(L->getHeader(), PSI, BFI);
   if (Hints.getForce() != LoopVectorizeHints::FK_Enabled &&
-      (F->hasOptSize() ||
-       llvm::shouldOptimizeForSize(L->getHeader(), PSI, BFI)))
+      (F->hasOptSize() || IsColdByProfile.getValueOr(false)))
     SEL = CM_ScalarEpilogueNotAllowedOptSize;
   else if (PreferPredicateOverEpilog ||
            Hints.getPredicate() == LoopVectorizeHints::FK_Enabled ||
            (TTI->preferPredicateOverEpilogue(L, LI, *SE, *AC, TLI, DT, LAI) &&
             Hints.getPredicate() != LoopVectorizeHints::FK_Disabled))
     SEL = CM_ScalarEpilogueNotNeededUsePredicate;
 
   return SEL;

llvm/unittests/Target/X86/MachineSizeOptsTest.cpp

TEST_F(MachineSizeOptsTest, Test) {
   auto iter = BB0.succ_begin();
   MachineBasicBlock *BB1 = *iter;
   iter++;
   MachineBasicBlock *BB2 = *iter;
   iter++;
   ASSERT_TRUE(iter == BB0.succ_end());
   MachineBasicBlock *BB3 = *BB1->succ_begin();
   ASSERT_TRUE(BB3 == *BB2->succ_begin());
-  EXPECT_FALSE(shouldOptimizeForSize(F, &PSI, MBFI_F));
-  EXPECT_TRUE(shouldOptimizeForSize(G, &PSI, MBFI_G));
-  EXPECT_FALSE(shouldOptimizeForSize(H, &PSI, MBFI_H));
-  EXPECT_FALSE(shouldOptimizeForSize(&BB0, &PSI, MBFI_F));
-  EXPECT_FALSE(shouldOptimizeForSize(BB1, &PSI, MBFI_F));
-  EXPECT_TRUE(shouldOptimizeForSize(BB2, &PSI, MBFI_F));
-  EXPECT_FALSE(shouldOptimizeForSize(BB3, &PSI, MBFI_F));
+  EXPECT_FALSE(shouldOptimizeForSize(F, &PSI, MBFI_F).getValueOr(false));
+  EXPECT_TRUE(shouldOptimizeForSize(G, &PSI, MBFI_G).getValueOr(false));
+  EXPECT_FALSE(shouldOptimizeForSize(H, &PSI, MBFI_H).getValueOr(false));
+  EXPECT_FALSE(shouldOptimizeForSize(&BB0, &PSI, MBFI_F).getValueOr(false));
+  EXPECT_FALSE(shouldOptimizeForSize(BB1, &PSI, MBFI_F).getValueOr(false));
+  EXPECT_TRUE(shouldOptimizeForSize(BB2, &PSI, MBFI_F).getValueOr(false));
+  EXPECT_FALSE(shouldOptimizeForSize(BB3, &PSI, MBFI_F).getValueOr(false));
 }
 
 const char* MachineSizeOptsTest::MIRString = R"MIR(
 --- |
   define i32 @g(i32 %x) !prof !14 {
     ret i32 0
   }
 

llvm/unittests/Transforms/Utils/SizeOptsTest.cpp

TEST_F(SizeOptsTest, Test) {
   BlockFrequencyInfo *BFI_G = BFID_G.get();
   BlockFrequencyInfo *BFI_H = BFID_H.get();
   BasicBlock &BB0 = F->getEntryBlock();
   BasicBlock *BB1 = BB0.getTerminator()->getSuccessor(0);
   BasicBlock *BB2 = BB0.getTerminator()->getSuccessor(1);
   BasicBlock *BB3 = BB1->getSingleSuccessor();
 
   EXPECT_TRUE(PSI.hasProfileSummary());
-  EXPECT_FALSE(shouldOptimizeForSize(F, &PSI, BFI_F));
-  EXPECT_TRUE(shouldOptimizeForSize(G, &PSI, BFI_G));
-  EXPECT_FALSE(shouldOptimizeForSize(H, &PSI, BFI_H));
-  EXPECT_FALSE(shouldOptimizeForSize(&BB0, &PSI, BFI_F));
-  EXPECT_FALSE(shouldOptimizeForSize(BB1, &PSI, BFI_F));
-  EXPECT_TRUE(shouldOptimizeForSize(BB2, &PSI, BFI_F));
-  EXPECT_FALSE(shouldOptimizeForSize(BB3, &PSI, BFI_F));
+  EXPECT_FALSE(shouldOptimizeForSize(F, &PSI, BFI_F).getValueOr(false));
+  EXPECT_TRUE(shouldOptimizeForSize(G, &PSI, BFI_G).getValueOr(false));
+  EXPECT_FALSE(shouldOptimizeForSize(H, &PSI, BFI_H).getValueOr(false));
+  EXPECT_FALSE(shouldOptimizeForSize(&BB0, &PSI, BFI_F).getValueOr(false));
+  EXPECT_FALSE(shouldOptimizeForSize(BB1, &PSI, BFI_F).getValueOr(false));
+  EXPECT_TRUE(shouldOptimizeForSize(BB2, &PSI, BFI_F).getValueOr(false));
+  EXPECT_FALSE(shouldOptimizeForSize(BB3, &PSI, BFI_F).getValueOr(false));
 }
 
 const char* SizeOptsTest::IRString = R"IR(
   define i32 @g(i32 %x) !prof !14 {
     ret i32 0
   }
 
   define i32 @h(i32 %x) !prof !15 {