@dcaballe Sorry couldnt remind earlier. Was on vacation myself. Did you need anything else for the review ?

Recommit fix that address the incorrect fusion shown in last commit

@dcaballe Thanks for the suggestion. I have added the example that shows the incorrect fusion. You see how the initialization of the reduction is merged into the reduction body due to bounds of the slice being set as 1.

Example showing fusion is incorrect after slice with incorrect bounds is inserted.

@vinayaka-polymage Thanks for taking a look! Perhaps I should have worded it differently. The result of the entire fusion is what am referring to here . While it is a valid fusion canidadate, what is incorrect is not using the entire slice bounds. A similar fixing of bounds
happens when there is read-read dependency. See https://github.com/llvm/llvm-project/blob/3e6ba89055c80e6360b7605464520711b30084a6/mlir/lib/Dialect/Affine/Analysis/Utils.cpp#L1107 . My change is along those lines and is keeping the origina bounds
when the producer is a memref based reduction. The pattern checking is checking if it is a memref based reduction like suggested in https://discourse.llvm.org/t/detecting-reduction-loops-in-affine-dialect/2527

Ping...

Update test

clang-format fix

Address code review comments

Address code review comments

• Input option as enum
• Producer Fusion with max src count

4 -way split that brings down the time per test from 5.5 seconds on a 16 core machine to about 1.5 second. The last file has some more room to grow.

Here is using time on a 16 core machine

In D106473#2895388, @rriddle wrote:

In D106473#2895386, @bondhugula wrote:

In D106473#2895372, @xgupta wrote:

mlir/test/transforms/loop-fusion.mlir is too big and is split into mlir/test/transforms/loop-fusion.mlir and mlir/test/transforms/loop-fusion-2.mlir

On which basis? Don't you think having _two similar names_ make confusion who don't know the context of test splitting.

This test case file has been slowing down check-mlir for everyone. I won't be surprised if this is the one that runs the longest across all test cases in the codebase. It has to be split. Open to a better/logical way of partitioning (for eg. sibling fusion vs producer consumer fusion) but they both might just be intertwined to separate in a balanced way. It is common for large files to be split this way - both sources and test cases. For eg. in the TF/MLIR repo, tf_ops.cc became too large and they went with something like t_ops_a_m.cc and tf_ops_n_z.cc to cut down the 60+s compile time.

The main problem for me is that this split feels rather arbitrary. The original file still has >3000 lines. Has any benchmarking gone into determining the split point/number of partitions/etc?

One asan fix

Fix a clang-format issue

FIx a clang-format issue

Thanks @bondhugula @vinayaka-polymage for the review and helpful comments!!

isReductionLoop->isLoopParallelAndContainsReductionLoop

Adrress review comments

Address comments

• Reflow all coments
  • Additional argument to promoteSingleIterReduction to control when operations are moved out

Add an extra clarification for when the promotion API can be safely used.

@vinayaka-polymage Thanks for additional comments. Can you pls check if my clarifications address your concerns?

Address comments

Reflow a comment

Reflow a comment

In D104249#2837879, @bondhugula wrote:

@sumesh13 Could you also please mention whether some of the methods were just moved without any change? That's useful in the commit summary.

Address code review comments including rename API to replaceForOpWithNewYields

Thansk for the additional comments.

Address review comments and add check to only do cleanup if the loop trip counts match.

Thanks for the feedback. I have addressed the review comments. Please take a look.

Address Code review comments

Address Code review comments

@bondhugula , @vinayaka-polymage thanks for the review. @sgrechanik Can you pls help land this

Address comment on simplifying condition to add ops with users.

Simplify condition to collect ops with users.

Gentle reminder..also pls suggest if it would be better to collect all the opsWithUsers separately as i was doing in a previous revision. For nested ifs, looks like otherwise you may not be collecting all the opsWithUsers by just collecting at the top level and leaves.

Adding back a change from previous revision that was approved and some minor cleanup.

Revert to earlier revision that was approved and some minor cleanup of comments.

@vinayaka-polymage Just noticed that you accepted a previous revision with comment on handling of ifops. Considering ifops, would you think it might be helpful to just collect all the ops like I was doing in a previous revision ?

Address comments

Mark review comments as done and undo a typo that had crept in

Undo a change w.r.t to the base that had crept in

@vinayaka-polymage Thanks for the comments. Pls take a look

Address review comments on code reorg

I have addresses the comments. Pls take a look.

In D99761#2675903, @vinayaka-polymage wrote:

Thank you for bringing up this issue in LICM, it is an important one. I have a few comments on the fix:

1. Inherent issue is definedOps ( going to be renamed to opsWithUsers) is being populated only under some conditions, and does not actually do the job of collecting all ops that define an SSA value in that block.
2. Even after this fix, only AffineForOps will be added to the list, but there are other cases like AffineDma... ops, AffineIfOps that are actually NOT code invariant etc. which will still cause similar problems.

So, I suggest that you can implement a generic fix for this inherent issue, where any op that defines an SSA value can be added to the above list, and this is decoupled from the checks like 'isOpLoopInvariant`. This will make the check more robust, and relevant test cases can also be added.

Rework based on comments

• Separate the part that identifies operations that have users and use it in the later phase.

Gentle ping

1. Address code review comments including renaming definedOps to opsWithusers