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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

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:

  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.



================
Comment at: lib/Analysis/ScopBuilder.cpp:2044-2046
+      // leadership status when merged into another statement. However, we
+      // don't need to get leader for 'Prev' everytime since the new leader of
+      // merged statements is still in 'SeenLeaders'.
----------------
Your new comment refers to the code before your patch, which will not be seen in the checkout anymore.


Repository:
  rPLO Polly

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

https://reviews.llvm.org/D67007