Verifier in loop pass manager

Hi,

When I recently was working on PR27157, I discovered a weird behavior in our verifier, which made it impossible to discover the bug by using ‘-verify-dom-info - verify’ flags. This happens because, when we fully unroll a loop, we remove it from the LPM loop queue along with all analyses related to it. Here is the code responsible for it:

{
PassManagerPrettyStackEntry X(P, *CurrentLoop->getHeader());
TimeRegion PassTimer(getPassTimer(P));

Changed |= P->runOnLoop(CurrentLoop, *this);
}
LoopWasDeleted = CurrentLoop->isInvalid();

if (Changed)
dumpPassInfo(P, MODIFICATION_MSG, ON_LOOP_MSG,
LoopWasDeleted ? “”
: CurrentLoop->getHeader()->getName());
dumpPreservedSet(P);

if (LoopWasDeleted) {
// Notify passes that the loop is being deleted.
deleteSimpleAnalysisLoop(CurrentLoop);
} else {
// Manually check that this loop is still healthy. This is done
// instead of relying on LoopInfo::verifyLoop since LoopInfo
// is a function pass and it’s really expensive to verify every
// loop in the function every time. That level of checking can be
// enabled with the -verify-loop-info option.
{
TimeRegion PassTimer(getPassTimer(&LIWP));
CurrentLoop->verifyLoop();
}

// Then call the regular verifyAnalysis functions.
verifyPreservedAnalysis(P);

F.getContext().yield();
}

Was it intentional that verifyPreservedAnalysis is under !LoopWasDeleted condition? Will anything break if we move verifyPreservedAnalysis out of the if?

Thanks,
Michael

Mikhail Zolotukhin <mzolotukhin@apple.com> writes:

Hi,

When I recently was working on PR27157, I discovered a weird behavior
in our verifier, which made it impossible to discover the bug by using
'-verify-dom-info - verify' flags. This happens because, when we fully
unroll a loop, we remove it from the LPM loop queue along with all
analyses related to it. Here is the code responsible for it:

      {
        PassManagerPrettyStackEntry X(P, *CurrentLoop->getHeader());
        TimeRegion PassTimer(getPassTimer(P));

        Changed |= P->runOnLoop(CurrentLoop, *this);
      }
      LoopWasDeleted = CurrentLoop->isInvalid();

      if (Changed)
        dumpPassInfo(P, MODIFICATION_MSG, ON_LOOP_MSG,
                     LoopWasDeleted ? "<deleted>"
                                    : CurrentLoop->getHeader()->getName());
      dumpPreservedSet(P);

      if (LoopWasDeleted) {
        // Notify passes that the loop is being deleted.
        deleteSimpleAnalysisLoop(CurrentLoop);
      } else {
        // Manually check that this loop is still healthy. This is done
        // instead of relying on LoopInfo::verifyLoop since LoopInfo
        // is a function pass and it's really expensive to verify every
        // loop in the function every time. That level of checking can be
        // enabled with the -verify-loop-info option.
        {
          TimeRegion PassTimer(getPassTimer(&LIWP));
          CurrentLoop->verifyLoop();
        }

        // Then call the regular verifyAnalysis functions.
        verifyPreservedAnalysis(P);

        F.getContext().yield();
      }

Was it intentional that verifyPreservedAnalysis is under
!LoopWasDeleted condition? Will anything break if we move
verifyPreservedAnalysis out of the if?

Seems like a mistake to me. Try it?

Mikhail Zolotukhin <mzolotukhin@apple.com> writes:

Hi,

When I recently was working on PR27157, I discovered a weird behavior
in our verifier, which made it impossible to discover the bug by using
‘-verify-dom-info - verify’ flags. This happens because, when we fully
unroll a loop, we remove it from the LPM loop queue along with all
analyses related to it. Here is the code responsible for it:

{
PassManagerPrettyStackEntry X(P, *CurrentLoop->getHeader());
TimeRegion PassTimer(getPassTimer(P));

Changed |= P->runOnLoop(CurrentLoop, *this);
}
LoopWasDeleted = CurrentLoop->isInvalid();

if (Changed)
dumpPassInfo(P, MODIFICATION_MSG, ON_LOOP_MSG,
LoopWasDeleted ? “”
: CurrentLoop->getHeader()->getName());
dumpPreservedSet(P);

if (LoopWasDeleted) {
// Notify passes that the loop is being deleted.
deleteSimpleAnalysisLoop(CurrentLoop);
} else {
// Manually check that this loop is still healthy. This is done
// instead of relying on LoopInfo::verifyLoop since LoopInfo
// is a function pass and it’s really expensive to verify every
// loop in the function every time. That level of checking can be
// enabled with the -verify-loop-info option.
{
TimeRegion PassTimer(getPassTimer(&LIWP));
CurrentLoop->verifyLoop();
}

// Then call the regular verifyAnalysis functions.
verifyPreservedAnalysis(P);

F.getContext().yield();
}

Was it intentional that verifyPreservedAnalysis is under
!LoopWasDeleted condition? Will anything break if we move
verifyPreservedAnalysis out of the if?

Seems like a mistake to me. Try it?

It doesn’t break 'ninja check’, but that doesn’t tell much. I can do more testing and then commit if that sounds reasonable. But there is at least one possible argument against it: we’ll verify all analyses after every loop, which is expensive (there is even a comment about it).

Thinking about it more - maybe the problem isn’t here. If we broke DT, we should be able to detect when LPM is finished, but we don’t. It looks like LPM just doesn’t state that DT is preserved. Here is how LPPassManager::getAnalysisUsage looks now:

void LPPassManager::getAnalysisUsage(AnalysisUsage &Info) const {
// LPPassManager needs LoopInfo. In the long term LoopInfo class will
// become part of LPPassManager.
Info.addRequired();
Info.setPreservesAll();
}

Would it make sense to add

Info.addRequired();
Info.addPreserved();
there? I don’t know, what ‘setPreservesAll’ is supposed to do, but adding these two lines do have an effect that I’m looking for: pass manager starts running verifyDomTree after the loop pass manager.

Thanks,
Michael