This is an archive of the discontinued LLVM Phabricator instance.

[LoopFusion] Move instructions from FC0.Latch to FC1.Latch.
ClosedPublic

Authored by Whitney on Dec 7 2019, 5:26 AM.

Details

Summary

This PR move instructions from FC0.Latch bottom up to the beginning of FC1.Latch as long as they are proven safe.

To illustrate why this is beneficial, let's consider the following example:
Before Fusion:

header1:
  br header2
header2:
  br header2, latch1
latch1:
  br header1, preheader3
preheader3:
  br header3
header3:
  br header4
header4:
  br header4, latch3
latch3:
  br header3, exit3

After Fusion (before this PR):

header1:
  br header2
header2:
  br header2, latch1
latch1:
  br header3
header3:
  br header4
header4:
  br header4, latch3
latch3:
  br header1, exit3

Note that preheader3 is removed during fusion before this PR.
Notice that we cannot fuse loop2 with loop4 as there exists block latch1 in between.
This PR move instructions from latch1 to beginning of latch3, and remove block latch1. LoopFusion is now able to fuse loop nest recursively.
After Fusion (after this PR):

header1:
  br header2
header2:
  br header3
header3:
  br header4
header4:
  br header2, latch3
latch3:
  br header1, exit3

Diff Detail

Event Timeline

Whitney created this revision.Dec 7 2019, 5:26 AM

Is there a mistake in the example? Every execution will end in the infinite loop header4. Why did you mean to move preheader3 instead of latch1?

Whitney edited the summary of this revision. (Show Details)Dec 7 2019, 2:58 PM
Whitney edited the summary of this revision. (Show Details)
Whitney edited the summary of this revision. (Show Details)Dec 7 2019, 3:00 PM

Is there a mistake in the example? Every execution will end in the infinite loop header4. Why did you mean to move preheader3 instead of latch1?

Sorry for the confusion, updated the description. The terminator of header4 should also branch to latch3. preheader3 is removed during fusion before this PR, updated the description to clarify that.

Whitney edited the summary of this revision. (Show Details)Dec 7 2019, 3:05 PM
Whitney edited the summary of this revision. (Show Details)

I'm generally OK with this. Two comments below, otherwise assume I'm fine with it, I mean no need to wait for me.

We miss a negative test. I mean one with instruction(s) that cannot be moved.

I find it odd to add the same code twice. Please put it in helper functions.

llvm/test/Transforms/LoopFusion/loop_nest.ll
36

Nit: It might be good to commit the renaming of the instructions and the checking for them before as a NFC commit (no need to ask for a review).

Whitney added a comment.EditedDec 9 2019, 12:24 PM

We miss a negative test. I mean one with instruction(s) that cannot be moved.

Will create a new test.

I find it odd to add the same code twice. Please put it in helper functions.

In https://reviews.llvm.org/D65464, @kbarton said he will clean up the similar code between performFusion() and fuseGuardedLoops() in later patches, that's why I left it as duplication. I will still put my new code in helper functions anyways.

Whitney updated this revision to Diff 232947.Dec 9 2019, 3:07 PM

Addressed Johannes's comments. Will do the NFC commit to rename instructions in one of the LITs after https://reviews.llvm.org/D71025 is landed.

Meinersbur accepted this revision.Dec 16 2019, 1:51 PM

LGTM

llvm/lib/Transforms/Scalar/LoopFuse.cpp
1134

[style]

for (Instruction &I : drop_begin(reverse(FromBB), 1))
1135

[suggestion] Can this be hoisted before the loop?

1139–1142

[style] https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

if (!isSafeToMoveBefore(I, *MovePos, DT, PDT, DI))
  break;

I.moveBefore(MovePos);
This revision is now accepted and ready to land.Dec 16 2019, 1:51 PM
Whitney marked 3 inline comments as done.Dec 16 2019, 5:13 PM
Whitney added inline comments.
llvm/lib/Transforms/Scalar/LoopFuse.cpp
1134

It need to be incremented before moveBefore as it move instructions out of FromBB.

1135

Is intentionally putted inside, ToBB.getFirstNonPHIOrDbg() returns a different value every iteration, as it will be the previous moved instruction.

1139–1142

I will actually remove the else break; as we want to move as much safe instructions as possible.

Whitney updated this revision to Diff 234210.Dec 16 2019, 5:58 PM

Move moveInstsBottomUp to CodeMoverUtils.

Meinersbur accepted this revision.Dec 17 2019, 12:30 PM
Meinersbur added inline comments.
llvm/lib/Transforms/Scalar/LoopFuse.cpp
1134

Could you add a comment about why the Iterator is incremented where it is? I usually try to avoid iterator magic.

Whitney updated this revision to Diff 234377.Dec 17 2019, 1:13 PM

Addressed Michael and Kit's comments.

kbarton accepted this revision.Dec 17 2019, 1:24 PM

LGTM.

This revision was automatically updated to reflect the committed changes.