- User Since
- Feb 18 2021, 8:51 PM (11 w, 6 d)
Sat, May 8
Adding why the issue was not discovered till now
Better commit summary and message
Wed, May 5
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
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
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
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.