[PATCH] D70482: [NFC] Change return type for 'shouldOptimizeForSize'

[David Tellenbach via Phabricator via llvm-commits]

tellenbach added a comment.

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!



================
Comment at: llvm/include/llvm/CodeGen/MachineSizeOpts.h:27
+/// base on the profile, false if not. Returns None if there is no info the
+/// profile.
+Optional<bool> shouldOptimizeForSize(const MachineFunction *MF,
----------------
Some word ist missing here: "if there is no info ? the profile"


================
Comment at: llvm/include/llvm/CodeGen/MachineSizeOpts.h:33
+/// base on the profile, false if not. Returns None if there is no info the
+/// profile.
+Optional<bool> shouldOptimizeForSize(const MachineBasicBlock *MBB,
----------------
Same here: Missing word in comment.


================
Comment at: llvm/include/llvm/Transforms/Utils/SizeOpts.h:39
+                                             ProfileSummaryInfo *PSI,
+                                             BFIT *BFI) {
   assert(F);
----------------
Please clang-format the whole signature. I think this would move `BFIT *BFI) {` one line up.


================
Comment at: llvm/include/llvm/Transforms/Utils/SizeOpts.h:78
 /// Returns true if function \p F is suggested to be size-optimized base on the
-/// profile.
-bool shouldOptimizeForSize(const Function *F, ProfileSummaryInfo *PSI,
-                           BlockFrequencyInfo *BFI);
+/// profile, false if not. Returns None if there is no info the profile.
+Optional<bool> shouldOptimizeForSize(const Function *F, ProfileSummaryInfo *PSI,
----------------
Same as above: Missing word in comment.


================
Comment at: llvm/include/llvm/Transforms/Utils/SizeOpts.h:83
 /// Returns true if basic block \p BB is suggested to be size-optimized base
-/// on the profile.
-bool shouldOptimizeForSize(const BasicBlock *BB, ProfileSummaryInfo *PSI,
-                           BlockFrequencyInfo *BFI);
-
+/// on the profile, false if not. Returns None if there is no info the profile.
+Optional<bool> shouldOptimizeForSize(const BasicBlock *BB,
----------------
Same as above: Missing word in comment.


================
Comment at: llvm/include/llvm/Transforms/Utils/SizeOpts.h:86
+                                     ProfileSummaryInfo *PSI,
+                                     BlockFrequencyInfo *BFI);
 } // end namespace llvm
----------------
Please clang-format the whole signature.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70482/new/

https://reviews.llvm.org/D70482