[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#1658185 <https://reviews.llvm.org/D67007#1658185>, @Meinersbur wrote:

> Ahh, I think I understand the argument. Thank you for your patience. But I strongly suggest to update the description. Suggestion:
>
>   // We are backtracking from the last element until we see Inst's leader in SeenLeaders and merge all into one set. Although which instructions are leaders changes during the execution of this loop, it is irrelevant as we are just searching for the element that we already confirmed is in the list.
>    for (Instruction *Prev : reverse(SeenLeaders)) {
>   
>
> If `SetVector.insert` would have returned an iterator to the found element, we could also have forward iterated from that element to the end.
>
> Maybe also add a comment to `bool Inserted = SeenLeaders.insert(Leader)` such as:
>
>   // Since previous iterations might have merged sets, some items in SeenLeaders are not leaders anymore. However, former leaders represent the same set as the actually leader also in SeenLeaders and by construction are all trailing the list.
>
>
> Since apparently its even hard for myself (who wrote this algorithm) to re-understand what's going on.


How about below comments?  I made your suggested comment a bit easier for me to understand (my bad English!).  Also removed the trailing words since previously merged groups could be not trailing in SeenLeaders.

  Instruction *Leader = UnionFind.getLeaderValue(Inst);
  // Since previous iterations might have merged sets, some items in
  // SeenLeaders are not leaders anymore. However, The new leader of
  // previously merged instructions must be one of the former leaders
  // of these merged instructions.
  bool Inserted = SeenLeaders.insert(Leader);
  if (Inserted)
    continue;
  
  // Merge statements to close holes. Say, we have already seen statements A
  // and B, in this order. Then we see an instruction of A again and we would
  // see the pattern "A B A". This function joins all statements until the
  // only seen occurrence of A.
  for (Instruction *Prev : reverse(SeenLeaders)) {
    // We are backtracking from the last element until we see Inst's leader
    // in SeenLeaders and merge all into one set. Although leaders of
    // instructions change during the execution of this loop, it's irrelevant
    // as we are just searching for the element that we already confirmed is
    // in the list.
    if (Prev == Leader)
      break;
    UnionFind.unionSets(Prev, Leader);
  }

Does this look fine?

Thanks,


Repository:
  rPLO Polly

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

https://reviews.llvm.org/D67007