User Details
- User Since
- Feb 18 2021, 8:51 PM (65 w, 3 d)
Jan 12 2022
Nov 29 2021
Looks good, minor request to complicate the test case, thanks!
Oct 7 2021
Simple clarification.
Sep 29 2021
Aug 25 2021
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.
Aug 2 2021
Looks good now, thanks for addressing the comments.
Thanks for the clarification, will take another look in some time.
Aug 1 2021
Thanks for this enhancement!
Jul 25 2021
Thanks @tungld ! It looks good to me.
Jul 23 2021
Thank you, changes look good. Can you please add a brief summary for the fix (unless you think it is generally obvious) ?
Jul 22 2021
Thanks for addressing all the comments. Changes look good!
Jul 21 2021
Thanks for this, occurrence an AffineIfOp doesn't need to prevent fusion happening elsewhere, and canFuseLoops already checks for AffineIfOps.
Jul 15 2021
Looks good, minor documentation nits.
Jul 14 2021
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.
Jul 13 2021
Thanks for addressing all the comments, I will make a final pass on it in a day.
Jul 12 2021
Suggestion
Jul 11 2021
Changes look good now, minor clarification request.
Jul 5 2021
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.
Jun 28 2021
Thanks for addressing comments, sorry for getting back on this so late.
Jun 22 2021
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 }
Jun 21 2021
Jun 20 2021
Jun 16 2021
Thanks for this, I am posting comments based on a very superficial glance, I will go through it in more detail.
- I have posted some minor inline comments.
- 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 .
Jun 11 2021
Looks great. Minor comments -
Jun 3 2021
Very helpful! Looks great, thanks.
May 24 2021
Incorporated all the suggestions, thanks @bondhugula !
Thanks for the suggestions for compactness, all of them worked well.
May 23 2021
May 17 2021
Addressed typos.
Thanks for the quick review, corrected the typos, and added additional
comments for clarification.
May 8 2021
Adding why the issue was not discovered till now
Better commit summary and message
May 5 2021
Looks great, thanks for fixing this hard and non-trivial bug !
Apr 12 2021
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 ?
Apr 11 2021
@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.
Apr 7 2021
Minor suggestion.
Thank you for bringing up this issue in LICM, it is an important one. I have a few comments on the fix:
- 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.
- 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.
Mar 31 2021
Recent suggestions incorporated. I have asked for clarification on one comment.
Addressing minor comments.
Addressing recent comments, minor.
Mar 27 2021
I have addressed the remaining comments:
- Instead of not printing the slice at all if it is incorrect, this change prints them but with a remark : "Incorrect slice" during testing.
- 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.
Mar 11 2021
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.
Mar 9 2021
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.
Feb 25 2021
Addressing the getOperandTypes comment
Thanks a lot for the insights @bondhugula and @dcaballe ! No more changes needed for this patch then.
Feb 24 2021
Looks good, thanks !
Feb 22 2021
Suggested additional checks and test-cases submitted at https://reviews.llvm.org/D97252
Test case fix.