Transformation: Loop Invariant Code Motion #3027
Conversation
RRavikiran66
changed the title
|
Oh actually the documentation lies about the side effects check, that's done outside of the helper in CPP, we might as well follow their logic for now. |
I have included your changes in the new commit but I forgot rename the canBeHoisted I will do it in the next commit. I have implemented helper functions like |
|
Can you please fix all the linting errors? Running |
RRavikiran66
changed the title
| # is_side_effect_free, | ||
| # only_has_effect, |
There was a problem hiding this comment.
Please let the person who commented resolve the conversation themselves in the PR. Why did you resolve my comment? We generally try to not have any commented code in the xDSL code base, so I would recommend deleting the above two lines.
There was a problem hiding this comment.
I resolved them, Cause I thought running precommit will remove unnecessary imports which I realized is not true for commented code, I didn't look for it either. I will be cautious of this next time
tests/filecheck/transforms/loop-hoist-memref/loop-invariant-code-motion.mlir
Outdated
Show resolved
Hide resolved
tests/filecheck/transforms/loop-hoist-memref/loop-invariant-code-motion.mlir
Outdated
Show resolved
Hide resolved
tests/filecheck/transforms/loop-hoist-memref/loop-invariant-code-motion.mlir
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3027 +/- ##
==========================================
+ Coverage 89.83% 89.86% +0.03%
==========================================
Files 411 417 +6
Lines 51698 52546 +848
Branches 8036 8144 +108
==========================================
+ Hits 46442 47220 +778
- Misses 3970 4011 +41
- Partials 1286 1315 +29 ☔ View full report in Codecov by Sentry. |
|
I will create three PR for rest of the Helper Functions |
| def canBeHoisted(op: Operation, region_target: Region) -> bool | None: | ||
| # Do not move terminators. | ||
| if op.has_trait(IsTerminator): | ||
| return False | ||
|
|
||
| # Walk the nested operations and check that all used values are either | ||
| # defined outside of the loop or in a nested region, but not at the level of | ||
| # the loop body. | ||
| for child in op.walk(): | ||
| for operand in child.operands: | ||
| for own in operand.owner.walk(): | ||
| if not isinstance(own, scf.For): | ||
| if op.is_ancestor(operand.owner): | ||
| continue | ||
| if region_target.is_ancestor(own): | ||
| return False | ||
| return True |
There was a problem hiding this comment.
Could you please first open a PR just for this helper? This is the key piece of logic in this PR, and it would be great to have a separately tested function that we could discuss. There are a number of changes that I would recommend making to this PR, but it feels like these should be as part of a focused discussion.
There was a problem hiding this comment.
It may seem like more PRs will mean that merging it takes more time but it will almost certainly take less time overall to merge the changes.
There was a problem hiding this comment.
I don't understand few things:
1)If I create a helper function, How can I test the helper function without calling in a class?
2)The test setup should also be a part of this PR right?
There was a problem hiding this comment.
I would recommend adding a pytest test for the function specifically, it's saved me a few times when writing tricky code, as it's easier to pinpoint the error than with the filecheck. In this case, I would recommend constructing a loop nest manually, using a builder, and checking that all the branches are covered, in a file like tests/transforms/test_loop_invariant_code_motion.py.
|
I had a chat with Ravikiran, and he's currently working towards a paper with a deadline on the 22 of November, will wait until then to update this. |
This pull request initiates the development of loop invariant code motion (LICM) optimizations. I have implemented helper functions like
canBeHoisted,move_out_of_the_region, 'isMemoryeffectfree'. I am not sure about speculatable so for now it contains same body as 'isMemoryeffectfree' I think it is working for scf.for, and I have added few tests cases to verify as well.