Perhaps better to default to false, do robust performance testing and flip the option. You've mentioned code size reduction in one binary in the description, but good to validate there are no performance regressions in spec.

This function looks very odd to me. Why not separate it into shouldOptimizeForSize(Function *, BFI) and shouldOptimizeForSize(BasicBlock*,...)? All but one uses of this function passes a BB in which case the function is irrrelevant.

This check is not needed as isFunctionColdInCallGraph first checks if summary is available.

I am not convinced this shouldOptimizeForSize belongs to ProfileSummaryInfo but I don't have any good suggestions. One possibility is to keep the flag in PassBuilder and pass it to individual passes which then check the boolean and isFunctionColdInCallGraph. Or perhaps add a pass that annotates OptForSize attribute on functions based on profile data? (that will still not help with the BB version of this function). Thoughts?

This check is not needed as isFunctionColdInCallGraph first checks if summary is available.

True. I'd prefer having it here to make it clearer that we only do this with PGO. But it can go either way here.

I am not convinced this shouldOptimizeForSize belongs to ProfileSummaryInfo but I don't have any good suggestions. One possibility is to keep the flag in PassBuilder and pass it to individual passes which then check the boolean and isFunctionColdInCallGraph. Or perhaps add a pass that annotates OptForSize attribute on functions based on profile data? (that will still not help with the BB version of this function). Thoughts?

I agree - this may not belong to ProfileSummaryInfo. I think I put it there because it's tied to the existence of PSI and might potentially need access to some PSI-internal data or have opportunities for code sharing.

How about having a separate file like ProfileGuidedSizeOpt.h/cpp which hosts the flag and shouldOptimizeForSize, and more? I think that would have an advantage of being able to share the common code in a single place (calling isFunctionColdInCallGraph, etc.) and being less intrusive (not needing to change many pass constructors, etc.), and being able to handle the block-level profiles (unlike the OptForSize attribute).

I'm not sure which directory it could be in, as that would have an implication on the cross-directory dependencies (eg. if we want to use PSI/BB/MBB/BFI/MBFI as a parameter type in it, it must be somewhere above IR/Analysis/CodeGen yet accessible from Transform/CodeGen.). Is there some directory where this sort of files can be in?

The previous flavor of this function doesn't check !PSI->hasProfileSummary(). In both the versions, it is not necessary as isColdBlock and isFunctionColdInCallGraph return false if there is no summary. But if you choose to add the check, then do it in both the flavors of this function.

You don't need a separate run with explicit -pgso=false. You can use the first run and use CHECK and CHECK-LABEL where you use NPGSO and NPGO-LABEL . That's what I had in mind, but if you prefer this, it's fine.

This revision causes a traceback when compiling SPEC2017 523.xalanbmk_r with -O3 -m64 -mcpu=power9 -flto and PGO, on the -fprofile-use compile step for XMLDateTime.cpp. Can you please take a look? Let me know if don't have access to SPEC0217 source and need a reproducer. The traceback was as follows: