[llvm-dev] Verifier in loop pass manager

[Michael Zolotukhin via llvm-dev]

> On Apr 8, 2016, at 6:08 PM, Justin Bogner <jbogner at apple.com> wrote:
> 
> Mikhail Zolotukhin <mzolotukhin at apple.com <mailto:mzolotukhin at 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?
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<LoopInfoWrapperPass>();
  Info.setPreservesAll();
}

Would it make sense to add 
  Info.addRequired<DominatorTreeWrapperPass>();
  Info.addPreserved<DominatorTreeWrapperPass>();
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


> 
>> Thanks,
>> Michael

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160408/0f8fcdc9/attachment.html>