[PATCH] [LoopInterchange] Add support to interchange loops with reductions.

[Karthik Bhat]

Hi Renato,
Thanks for looking into the patch. Please find my comments inline.

> Wouldn't this be better to be integrated into the vectorizer? Or at least, not duplicated code from the vectorizer?


Since this is a complete pass integrating this will loop vectorizer would be a bit difficult.  I can try to move common functions into a utility file if required.

> Also, you have at least two other bold moves:

> 

> 1. Move a lot of internal methods to static. This could be ok, but needs some further explanation why it is so.


These methods are helper functions. Helper functions are usually marked static in other Passes i had missed it in my initial commit hence corrected it here.

> 2. Move the interchange pass down the pipeline. Again, it could be the right thing to do, but also needs some context.


I had to move interchange pass down the pipeline and add licm and loop unswitch pass after loop interchange as after the inner loop header/loop latches are split and loops are interchanged we get multiple blocks with successors outside the loop(i.e. getExitingBlock was null). This kind of loop is not handled by vectorize and hence had to run licm and loop unswitch to remove unconditional branches between blocks in the inner loop.

> Finally, how did you test and benchmark your changes?


I have run llvm lnt test cases but unfortunately didn't observe much improvement with this patch. I'm planning to run phoronix test suites to see if it gives improvement in some known benchmark. The matrix multiplication code was from one of our internal test cases which showed improvement post this patch. Have added few test cases (positive and negative) and ran make check all to make sure there are no regression. Also have written few local C test cases to check that the o/p's are same after interchange.

> 1. Run the "make check-all" tests and make sure they're green


Yes make check all passes with this patch. Have also added new test cases to test this feature.

> 2. Run the test-suite and make sure it passes *and* doesn't regress performance

> 3. Provide more information about the benchmarks that you tested it, relative improvements, regressions, etc.


Yes ran llvm lnt benchmark but didn't observe much improvement. No regressions were observed either. I have observed one crash in Dependency Analysis module which is triggered after this patch. Since this pass is currently disabled by default I was planning to address the crash in  Dependency Analysis module separately. Will that be ok?

I will try to get more benchmark data using phoronix test suites over the weekend and get back to you.

Thanks once again for looking into the patch. Looking forward to your inputs on the patch.
Regards
Karthik Bhat


http://reviews.llvm.org/D8314

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/