llvm dev - Dec 2009 - [LLVMdev] Preserving ProfileInfo in several Passes

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm-r90454.profileinfo-preservation.patch
Type: text/x-diff
Size: 67672 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20091203/22e0c60c/attachment.patch>

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

On Dec 3, 2009, at 9:56 AM, Andreas Neustifter wrote:
> 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
> 
>
<llvm-r90454.profileinfo-preservation.patch>_______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

Hi!

On 12/03/2009 07:50 PM, Dan Gohman wrote:
 > 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