r198640 - CodeGen: Initial instrumentation based PGO implementation

Eric Christopher echristo at gmail.com
Mon Jan 13 14:52:56 PST 2014


>> It would be great to see a top level description of the algorithm you're using
>> to do region creation etc along with an example. It seems that you're creating
>> regions from a way different than most previous literature on the topic so a
>> literature link would be good if you've got one, else a description is going
>> to be key.
>
> I am planning on providing a write up about all of this, but I
> obviously haven't done so yet. Rest assured, I haven't forgotten.

Would be good. Will be better to see it in the code too :)

>>
>>     -  // If the body of the case is just a 'break', and if there was no
>>     fallthrough,
>>     -  // try to not emit an empty block.
>>     -  if ((CGM.getCodeGenOpts().OptimizationLevel > 0) &&
>>     +  // If the body of the case is just a 'break', try to not emit an empty
>>     block.
>>     +  // If we're profiling or we're not optimizing, leave the block in for
>>     better
>>     +  // debug and coverage analysis.
>>     +  if (!CGM.getCodeGenOpts().ProfileInstrGenerate &&
>>     +      CGM.getCodeGenOpts().OptimizationLevel > 0 &&
>>            isa<BreakStmt>(S.getSubStmt())) {
>>          JumpDest Block = BreakContinueStack.back().BreakBlock;
>>
>> Is it really worth that much for code gen/optimization time to not just always
>> have it? Also, do we really want instrumentation to change the cfg we emit?
>> Also, it doesn't change in debug mode, just without optimization so that part
>> of the comment isn't quite accurate :)
>
> Well, we can't elide the block if we're profiling, since it won't be
> empty in that case (the instrumentation code has to go somewhere!). It
> looks like the original change to skip the empty block went in to clean
> up -O0 CodeGen (r129652) but that made things messier for debugging and
> for gcov (PR9796) so conditionalizing on OptimizationLevel > 0 was added
> in r154420.
>
> Presumably at anything above -O0 the backend will remove the empty block
> anyway, so it's probably safe to remove this logic completely. Thoughts?
>

I agree, but sometimes Chris gets a little funny about these extra
blocks. Chris?

-eric



More information about the cfe-commits mailing list