[PATCH] D41059: [PGO] MST min edge selection heuristic to ensure non-zero entry count

[Rong Xu via llvm-commits]

On Fri, Dec 15, 2017 at 2:48 PM, David Li via Phabricator
<reviews at reviews.llvm.org> wrote:
> davidxl added inline comments.
>
>
> ================
> Comment at: lib/Transforms/Instrumentation/CFGMST.h:183
> +    uint64_t EntryInWeight = EntryWeight;
> +    if (EntryInWeight < MaxExitOutWeight &&
> +        MaxEntryOutWeight < MaxExitInWeight)
> ----------------
> xur wrote:
>> Will this ever be true? EntryInWeight should be always greater than or equal to weight of Exit BB.
> Probably not -- will remove.
>
>
> ================
> Comment at: lib/Transforms/Instrumentation/CFGMST.h:198
> +    }
>    }
>
> ----------------
> xur wrote:
>> Do we need to make sure EntryInWeight less than MaxEntryOutWeight?
>>
>> If Entry BB has more than one successors,  and Exit BB has more than one predecessors,
>> even after adjustment, EntryIncoming->Weight might be greater than ExitOutgoing->Weight.
>>
>> EntryIncoming ExitOutgoing and ExitIncoming will be in MST (thus not instrumented).
>> EntryOutgoing  will be instrumented. But since it has two successors, we will instrumented the target BB.
>> (i.e. entryBB is not instrumented).
>>
>> But I'm sure  how this relates to the problem this patch wants to solve. It seems to me that choice b/w instrumenting entry and exit only applies the case of single successor/predecessor for entry and exit nodes.
> EntryIncomingWeight is adjusted to be smaller than MaxExitOutgoingWeight.
>
> In your case, all  EntryOutgoingEdges will be selected for instrumentation, and their sum will be the entry count.  Unless One of entry outgoing edges is a critical edge which will be selected as Min(ST) edge, in which case, the entryIncomingEdge won't need to be in MST and therefore will be instrumented. That is why we need to make sure entryIncoming weight is smaller than exitoutoging weight.

what I meant was here you are not instrumenting the Entry BB -- you
are instrumenting the successors of the entry BB.  And the successors
might be a non-return in which case, the result is still not good.

>
>
> https://reviews.llvm.org/D41059
>
>
>