Looks good, minor request to complicate the test case, thanks!

Simple clarification.

Hi @arjunp @Groverkss , sorry - I am currently unavailable until the end of the week, and will not be able to review this. I will check this during the weekend if it is still open.

Looks good now, thanks for addressing the comments.

Thanks for the clarification, will take another look in some time.

Thanks for this enhancement!

Thanks @tungld ! It looks good to me.

Thank you, changes look good. Can you please add a brief summary for the fix (unless you think it is generally obvious) ?

Thanks for addressing all the comments. Changes look good!

Thanks for this, occurrence an AffineIfOp doesn't need to prevent fusion happening elsewhere, and canFuseLoops already checks for AffineIfOps.

Looks good, minor documentation nits.

Thanks for addressing all the comments, sorry once again for taking so long to review. As some ops were moving out from one for op to its parent, I wanted to closely review the scenarios.

Thanks for addressing all the comments, I will make a final pass on it in a day.

Suggestion

Changes look good now, minor clarification request.

It seems correct to me, that the constant bound is non-negative always, on principle. Was this bound being negative creating any problem in some cases ? If yes, it might be useful to mention that in comments.

Thanks for addressing comments, sorry for getting back on this so late.

We can wait till tomorrow morning IST for any new comments.

I think, in summary, attempt here is to fuse two loops that are reducing under sibling fusion, which is good. I think, the conditions presented in this patch need to be stricter to make it work. Here is a counter example (output results in incorrect code):

func @reduce_add_f32_f32(%arg0: memref<64x64xf32, 1>, %arg1: memref<1x64xf32, 1>, %arg2: memref<1x64xf32, 1>) {
  %cst_0 = constant 0.000000e+00 : f32
  %cst_1 = constant 1.000000e+00 : f32
  %0 = memref.alloca() : memref<f32, 1>
  %1 = memref.alloca() : memref<f32, 1>
  affine.for %arg3 = 0 to 1 {
    affine.for %arg4 = 0 to 64 {
      %accum = affine.for %arg5 = 0 to 64 iter_args (%prevAccum = %cst_0) -> f32 {
        %4 = affine.load %arg0[%arg5, %arg4] : memref<64x64xf32, 1>
        %5 = addf %prevAccum, %4 : f32
        affine.yield %5 : f32
      }
      %accum_dbl = addf %accum, %accum : f32
      affine.store %accum_dbl, %arg1[%arg3, %arg4] : memref<1x64xf32, 1>
    }
  }
  affine.for %arg3 = 0 to 1 {
    affine.for %arg4 = 0 to 64 {
      %accum = affine.for %arg5 = 0 to 32 iter_args (%prevAccum = %cst_1) -> f32 { // modified the test case on this line 64 => 32
        %4 = affine.load %arg0[%arg5, %arg4] : memref<64x64xf32, 1>
        %5 = mulf %prevAccum, %4 : f32
        affine.yield %5 : f32
      }
      %accum_sqr = mulf %accum, %accum : f32
      affine.store %accum_sqr, %arg2[%arg3, %arg4] : memref<1x64xf32, 1>
    }
  }
  return
}

Thanks for this, I am posting comments based on a very superficial glance, I will go through it in more detail.

1. I have posted some minor inline comments.
2. It would be good to wrap the commit msg to the standard wrap width.

Looks good, saves PostDominanceInfo calculation.
Minor: comments at some places need to change to does not dominate any ... at a couple of places as pointed out by @ayzhuang .

Looks great. Minor comments -

Very helpful! Looks great, thanks.

Incorporated all the suggestions, thanks @bondhugula !

Thanks for the suggestions for compactness, all of them worked well.

Addressed typos.

Thanks for the quick review, corrected the typos, and added additional
comments for clarification.

Adding why the issue was not discovered till now

Better commit summary and message

Looks great, thanks for fixing this hard and non-trivial bug !

Please change the commit title to a more relevant one, I am approving this.

@sumesh13 Thanks for addressing all the comments, the change looks good now. Can you please check these last two comments before I approve this ?

@sumesh13 This new generic fix looks good, it handles more cases that were previously not handled. Thanks for this. I have added some comments related to code organization, please check them. Logic looks good to me.

Minor suggestion.

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.

Recent suggestions incorporated. I have asked for clarification on one comment.

Addressing minor comments.

Addressing recent comments, minor.

I have addressed the remaining comments:

1. Instead of not printing the slice at all if it is incorrect, this change prints them but with a remark : "Incorrect slice" during testing.
2. Fast check is being done, and is reusing the bounds checking utility function isSliceMaximalFastCheck. I have verified that with today's test file loop-fusion.mlir, of 156 calls to isSliceValid, 107 calls are returned after the this fast check itself.

Added back the slice computation remarks in tests, marked them incorrect slices.
Added a fast check, reused the utility - isSliceMaximalFastCheck.

Thanks for the review and suggestions !! I have completed the easy ones, and I will continue exploring the pending ones.

Addressing easy comments and suggestions, some important ones
still pending.

If this change/approach is acceptable, we can also change ComputationSliceState::isMaximal() to use the set difference the other way (source - slice), reusing these computations.

Addressing the getOperandTypes comment

Thanks a lot for the insights @bondhugula and @dcaballe ! No more changes needed for this patch then.

Looks good, thanks !

Suggested additional checks and test-cases submitted at https://reviews.llvm.org/D97252

Test case fix.