Page MenuHomePhabricator

[LoopFusion] Ensure that both loops are guarded or neither are guarded.
AcceptedPublic

Authored by kbarton on Dec 16 2019, 2:04 PM.

Details

Summary

This patch modifies the current conditions for loop fusion by ensuring that
either both fusion candidates have a guard, or neither have a guard. This
prevents us from inadvertantly attempting to fuse a guarded loop with a
non-guarded loop.

Event Timeline

kbarton created this revision.Dec 16 2019, 2:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 16 2019, 2:04 PM
Whitney added inline comments.Dec 18 2019, 3:43 PM
llvm/test/Transforms/LoopFusion/cannot_fuse.ll
421

optional:
define void @only_one_guarded(i32* noalias %A, i32* noalias %B, i1 zeroext %cond) {

Whitney added inline comments.Jan 21 2020, 7:11 PM
llvm/lib/Transforms/Scalar/LoopFuse.cpp
1070

haveIdenticalGuards() is not called with both candidates unguarded, so (!FC0.GuardBranch && !FC1.GuardBranch) is impossible.

LGTM. @Whitney?

llvm/lib/Transforms/Scalar/LoopFuse.cpp
738–739

[style] (FC0->GuardBranch || FC1->GuardBranch) && !haveIdenticalGuards(*FC0, *FC1)

1069–1070

The check can still make sense for defensive programming (or replaced by an assertion).

[style] !FC0.GuardBranch != !FC1.GuardBranch

LGTM. @Whitney?

I know that @kbarton is working on a patch which allow guarded loops fuse with unguarded loops as long as they are control flow equivalent.
I think that's a better solution. And with that, this patch is no longer needed.
However, I don't mind having this patch committed in the meantime.

Meinersbur accepted this revision.Feb 17 2020, 3:51 PM

OK then; Kit may either abandon or commit this patch.

This revision is now accepted and ready to land.Feb 17 2020, 3:51 PM

I wonder if this issue still exists in llvm trunk.

// Ensure that FC0 and FC1 have identical guards.
// If one (or both) are not guarded, this check is not necessary.
if (FC0->GuardBranch && FC1->GuardBranch &&
    !haveIdenticalGuards(*FC0, *FC1) && !TCDifference) {
  LLVM_DEBUG(dbgs() << "Fusion candidates do not have identical "
                       "guards. Not Fusing.\n");
  reportLoopFusion<OptimizationRemarkMissed>(*FC0, *FC1,
                                             NonIdenticalGuards);
  continue;
}

if (!isSafeToMoveBefore(*FC1->Preheader,
                        *FC0->Preheader->getTerminator(), DT, &PDT,
                        &DI)) {
  LLVM_DEBUG(dbgs() << "Fusion candidate contains unsafe "
                       "instructions in preheader. Not fusing.\n");
  reportLoopFusion<OptimizationRemarkMissed>(*FC0, *FC1,
                                             NonEmptyPreheader);
  continue;
}

if (FC0->GuardBranch) {
  assert(FC1->GuardBranch && "Expecting valid FC1 guard branch");

It seems we still need add checks to make sure both loops are guarded or neither are guarded. I enabled loop fusion in our compiler for Xtensa at Cadence, and encountered this issue (our llvm base is 10.0.1 at the moment).