[PATCH] D67007: [ScopBuilder]Skip getting leader when merging statements to close holes

[bin via Phabricator via llvm-commits]

bin.narwal added a comment.

In D67007#1655959 <https://reviews.llvm.org/D67007#1655959>, @Meinersbur wrote:

> Thank you for the find. Indeed the name of `PrevLeader` suggests it to be the leader of `Prev`. It's leader should be identical to leader to the last element of `SeenLeaders`, but only before the first `unionSet` operation. Then the tail ("B") has been merged into Inst's leader "A", making the condition a tautology. That's definitely wrong!
>
> However, I don't think we can save on the `getLeaderValue` call. Each of the instruction's leader may still be in `SeenLeader`, but the order in which it appears matter (it's a `SetVector`), and `Inst`'s leader may change as well. The most explicit version I can think of is:


Yes, the order is not guaranteed, and `Inst`'s leader could change during merging, but IIUC, these do not matter.
For the check condition:

  if (Prev != Leader)

"Leader" is computed when we try to insert "Inst" into SeenLeaders.  Whether leader of the "Leader" changes doesn't have impact on the loop, as long as the first "Prev" in the merging group is actually "Leader".  The cons is we may do unnecessary call to unionSets because we only break at the first Prev Inst of merging group.
So looks like we can either compare real leaders of the two Insts, or just compare the two Insts it-selves.

BTW, if SetVector supports truncate, we can truncate away Insts of merged group and push the new leader every time after the loop.  This way we don't need to check leaders, don't need to do unnecessary unionSets, and always keep SeenLeaders minimal.  But it's SetVector...
Thanks,

>   for (Instruction *Prev : reverse(SeenLeaders)) {
>     // Items added to 'SeenLeaders' are leaders, but may have lost their
>     // leadership status when merged into another statement.
>     Instruction *PrevLeader = UnionFind.getLeaderValue(Prev);
>     Instruction *InstLeader = UnionFind.getLeaderValue(Inst);
>     if (PrevLeader == InstLeader)
>       break;
>     UnionFind.unionSets(Prev, Leader);
>   }
>    
> 
> which also passes `granularity_scalar-indep_ordered-2.ll`. I don't think it's part of the API which of the leaders passed to `UnionFind.unionSets` becomes the new leader, so we have to check them both (unless you find an argument that one/both of the, are unnecessary).
> 
> The code above is the same as (which so far is my preferred version):
> 
>   for (Instruction *Prev : reverse(SeenLeaders)) {
>     if (UnionFind.isEquivalent(Prev, Inst))
>       break;
>     UnionFind.unionSets(Prev, Leader);
>   }
>    
> 
> We need regressions tests with more (potential) statements to see the difference and check that not all of them are merged.




Repository:
  rPLO Polly

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67007/new/

https://reviews.llvm.org/D67007