[llvm-commits] [PATCH 2/3] Remove workaround in PostDominators

[Tobias Grosser]

On 01/07/10 01:41, Dan Gohman wrote:
>
> On Jan 6, 2010, at 3:27 PM, Tobias Grosser wrote:
>
>>
>> Remove a FIXME and unify code that was necessary to work around broken
>> updateDFSNumbers().  Before updateDFSNumbers() did not work correctly for post
>> dominators.
>
> -
> -  // FIXME: This does not work on PostDomTrees.  It seems likely that this is
> -  // due to an error in the algorithm for post-dominators.  This really should
> -  // be investigated and fixed at some point.
> -  // DT.updateDFSNumbers();
> -
> -  // Start out with the DFS numbers being invalid.  Let them be computed if
> -  // demanded.
> -  DT.DFSInfoValid = false;
> +
> +  DT.updateDFSNumbers();
>
> Wouldn't it be better to do what the code was doing -- just set
> DFSInfoValid to false and let the value be recomputed lazily,
> instead of eagerly updating all the DFS numbers?

I thought about this, and timed it for a big program. The call of 
updateDFSNumber() took less then 5% of the time necessary for dominance 
tree calculation. As there was no big performance impact, I decided to 
just reinsert the code the original author commented out.

Currently updateDFSNumbers() is always called eagerly, but it is called 
in Dominators.h. With this patch I removed it from there, and 
reintroduce it where the original author put the comment.

Anyways, I do not have a strong opinion about that.

Tobi