[PATCH] D71689: Improve that the necessary and sufficient combination is not tried for multiple loops in loop-interchange

Fri Feb 28 13:24:18 PST 2020

fhahn added a comment.

@masakazu.ueno do you have any numbers on how many more loops are interchanged with this patch and/or runtime improvements for benchmarks (e.g. test-suite/MultiSource, SPEC2006)?

I think we should work towards pushing for LoopInterchange to be enabled by default and I think it is almost there. Any positive benchmark data would help. That being said, I think we should try to be rather conservative, so that we only interchange when we are relatively certain that there will be benefits. I think for example your case (1) will be quite dependent on the size of the last dimension and the caching hardware.

In D71689#1897557 <https://reviews.llvm.org/D71689#1897557>, @rengolin wrote:

> Nice patch, thanks!
>
> Some inline silly comments, but I have two mains comments.
>
> 1
>
> Today, we drop the search at the first sign of trouble. That's a bit silly, as you stated in your introduction.
>
> The new pattern is to only drop if it's illegal, but continue if it's not-profitable. I agree that this is guaranteed to be equal or better than the current scenario, result-wise. So, as is, looks like a clear improvement.

Agreed, it looks like this patch improves the current situation in a straight-foward way. I think it should be a good first step.

In the long term, I think we should re-structure the code to first compute the cost for each loop at the innermost position and move the best legal one to the innermost position. Repeat for the 2nd innermost level (and maybe further, although it's unlikely to really be beneficial).

> 2
> 
> The algorithm will now run for all legal combinations of the nesting structure, which is clearly equal or worse in terms of compiler time.
> 
> This is obviously expected, but it's still good to understand the impact. The combinations are exponential, but the number of nesting levels is usually small, so perhaps, not a big deal.
> 
> Maybe, as a safety measure (for pathological cases), we could limit the number of nesting loops we're allowed to inspect, just to be on the safe side.

+1, we should definitely limit the number of nested loops to something relatively small for now (5-10).

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

https://reviews.llvm.org/D71689