This is an archive of the discontinued LLVM Phabricator instance.

[LoopFusion] Restrict loop fusion to rotated loops.
ClosedPublic

Authored by kbarton on Dec 4 2019, 9:32 AM.

Details

Summary

This patch restricts loop fusion to only consider rotated loops as valid candidates.
This simplifies the analysis and transformation and aligns with other loop optimizations.

Event Timeline

kbarton created this revision.Dec 4 2019, 9:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 4 2019, 9:32 AM
Meinersbur accepted this revision.Dec 5 2019, 8:55 AM

[suggestion] Add a test case to check for "not rotated" to be rejected.

llvm/test/Transforms/LoopFusion/cannot_fuse.ll
270

Does this test something different now?

llvm/test/Transforms/LoopFusion/diagnostics_missed.ll
3–4

[nit] can be removed

This revision is now accepted and ready to land.Dec 5 2019, 8:55 AM
Whitney added inline comments.Dec 12 2019, 4:12 PM
llvm/test/Transforms/LoopFusion/cannot_fuse.ll
42

This guard was not in the original test case, now the second loop has two guards, is that intensional?

75

bb34 is also a predecessor of bb33.

kbarton marked 4 inline comments as done.Dec 16 2019, 9:02 AM

Addressed all the review comments.
I'm ready to land this, unless @Whitney has concerns about the test case.

llvm/test/Transforms/LoopFusion/cannot_fuse.ll
42

Yes, this is intentional to make the two loops not control-flow-equivalent, which is what this specific test is checking.

270

This is a good catch. I missed this. I've fixed it to check for the memory dependencies again.

Whitney added inline comments.Dec 16 2019, 9:39 AM
llvm/test/Transforms/LoopFusion/cannot_fuse.ll
42

The two loops should already be not control-flow-equivalent, as bb7 is not guarded, while bb22 is guarded. Why is the extra guard needed?

kbarton marked 2 inline comments as done.Dec 16 2019, 9:56 AM
kbarton added inline comments.
llvm/test/Transforms/LoopFusion/cannot_fuse.ll
42

If the loop is not guarded, the code will use the preheader; if the loop is guarded, it uses the guard.
In this case, since one is guarded and the other is not guarded, it will compare the preheader for the first loop with the guard for the second loop and find them control flow equivalent. It will fail in a later check for fusion, however this specific test is meant to check that loops that are not control flow equivalent are put into different candidate sets. Putting an extra branch around the second loop satisfies that.

Whitney added inline comments.Dec 16 2019, 10:08 AM
llvm/test/Transforms/LoopFusion/cannot_fuse.ll
42

Notice that before changing the test case to rotated form, there is only one loop guard needed. I think we should change the way fusion compute the control flow equivalent between two loops. As a guarded loop (assuming the condition not always true) and an unguarded loop should be consider not control flow equivalent. I am ok to keep that as future improvement.

kbarton marked an inline comment as done.Dec 16 2019, 10:43 AM
kbarton added inline comments.
llvm/test/Transforms/LoopFusion/cannot_fuse.ll
42

Yes, this is an existing problem that came up recently. I don't think the current patch exacerbates the problem though.
I am currently working on a fix for that, but would prefer to land this patch as is and fix the existing problem separately.

This revision was automatically updated to reflect the committed changes.