Hi all,
this (altough a big patch) is actually pretty straight forward: It (tries) to preserve ProfileInfo in all -std-compile-opts passes and all X86-Backend passes.
There is still some passes that have corner cases where the ProfileInfo is not correct after the pass. Some passes are still missing...
How shall I proceed with this?
Andi
Hello,
Here are a few misc. comments on this patch.
Would it make sense to mark the ProfileInfo passes as "CFGOnly?"
If so, that would let them be automatically preserved by passes
which don't modify the CFG (and that call AU.setPreservesCFG()).
+ if (ProfileInfo* PI = getAnalysisIfAvailable<ProfileInfo>()) {
+ PI->splitEdge(OrigPreHeader, NewHeader, NewPreHeader);
+ }
+
// Preserve canonical loop form, which means Exit block should
// have only one predecessor.
SplitEdge(L->getLoopLatch(), Exit, this);
Would it make sense to move the ProfileInfo updating code into
SplitEdge? That way all users of SplitEdge would automatically do
the right thing. Actually, SplitEdge just delegates to either
SplitCriticalEdge or SplitBlock, so those two should do the work.
In TailRecursionElimination.cpp, there's logic which uses a
"10-fold increase" heuristic. It would be nice if that code were
factored out into a utility routine so that the profiling
heuristics are more centralized.
- bool Folded = ConstantFoldTerminator(I->getParent());
+ bool Folded = ConstantFoldTerminator(I->getParent(), this);
I don't see any corresponding changes to the definition of
ConstantFoldTerminator. Are there files missing from the patch?
Dan
Hi!
> Hello,
>
> Here are a few misc. comments on this patch.
>
> Would it make sense to mark the ProfileInfo passes as "CFGOnly?"
> If so, that would let them be automatically preserved by passes
> which don't modify the CFG (and that call AU.setPreservesCFG()).
Yes, it would, how do I do this? 
>> + if (ProfileInfo* PI = getAnalysisIfAvailable<ProfileInfo>()) {
>> + PI->splitEdge(OrigPreHeader, NewHeader, NewPreHeader);
>> + }
>> +
>> // Preserve canonical loop form, which means Exit block should
>> // have only one predecessor.
>> SplitEdge(L->getLoopLatch(), Exit, this);
>
> Would it make sense to move the ProfileInfo updating code into
> SplitEdge? That way all users of SplitEdge would automatically do
> the right thing. Actually, SplitEdge just delegates to either
> SplitCriticalEdge or SplitBlock, so those two should do the work.
Yes, this is implemented already, but I have not removed the call to PI->SplitEdge(), in generall this patch is perliminary and my big question was how to deal with it commiting-wise, I still have to figure out some technical details. But thanks for the hint!
> In TailRecursionElimination.cpp, there's logic which uses a
> "10-fold increase" heuristic. It would be nice if that code were
> factored out into a utility routine so that the profiling
> heuristics are more centralized.
Agreed. Its hard to find all points that use such heuristics, since I couldn't find a common wording to grep on...
>> - bool Folded = ConstantFoldTerminator(I->getParent());
>> + bool Folded = ConstantFoldTerminator(I->getParent(), this);
>
> I don't see any corresponding changes to the definition of
> ConstantFoldTerminator. Are there files missing from the patch?
As mentioned earlier, I was more interested in the "HowTo" of handling this type of changes. But yes, the include/* changes are missing from this patch to keep it short and still make my point.
(Maybe this was not a good idea...).
Thanks for the comments!
Andi
It's one of the arguments to the RegisterPass constructor.
Dan