[PATCH] D22778: Add Loop Sink pass to reverse the LICM based of basic block frequency.

[Dehao Chen via llvm-commits]

danielcdh marked 13 inline comments as done.
danielcdh added a comment.

Thanks for the reviews!



> chandlerc wrote in LoopSink.cpp:148-170
> This seems really expensive. By my reading this is O(N * L * M) where L is the  number of basic blocks in a loop, N is the number of instructions we try to sink into that loop, and M is the number of basic blocks within the loop that use the instructions. If there is for example one hot basic block in the loop and a large number of cold basic blocks and all of the uses are in those cold basic blocks, it seems like this could become quite large.
> 
> Have you looked at other algorithms? Is there a particular reason to go with this one? (I've not thought about the problem very closely yet...)

I initially started with an adhoc algorithm which is O(L * M), but Danny pointed out it is not optimal, so I changed to this optimal algorithm. The lower bound for any sinking algorithm is O(L*M), but if optimal solution is desired, O(N*L*M) is the best I can get.

Yes, this could be expensive when N is large. I practice, I did not see noticeable compile time increase in speccpu2006 benchmarks after applying this patch (and enable the pass in frontend).

How about we limit the N to be no more than a certain number to avoid expensive computation in extreme cases?

> chandlerc wrote in LoopSink.cpp:214
> If this is the case we can't sink I at all though, right? I think that is what the code already does, maybe just update the comment?

Not sure if I get this right, do you mean update the comment (as I just did) to make it less redundant?

> chandlerc wrote in LoopSink.cpp:216
> Why not use `L->contains(UI)`?

Because it does not check for sub loops.

e.g.

Loop1 {

  I1
  Loop2 {
    I2
  }

}

Loop1->contains(I1) --> true
Loop2->contains(I2) --> true
Loop1->contains(I2) --> false

For this check we want to make sure I1 and I2 both return true.

https://reviews.llvm.org/D22778