This is an archive of the discontinued LLVM Phabricator instance.

[RegionInfo] Verify getRegionFor
ClosedPublic

Authored by Meinersbur on Aug 8 2015, 11:49 AM.

Details

Summary

Check the contents of BBtoRegion during analysis verification. It only takes place if -verify-region-info is passed or LLVM is compiled with XDEBUG.

RegionBase<Tr>::verifyRegion() also checks the RegionInfoBase<Tr>::VerifyRegionInfo flag, which is redundant, but verifyRegion() is public API and might be invoked from other sites. In order to avoid behavioral change, this check is not removed. In any case, no region will be verified unless VerifyRegionInfo is set.

Diff Detail

Repository
rL LLVM

Event Timeline

Meinersbur updated this revision to Diff 31591.Aug 8 2015, 11:49 AM
Meinersbur retitled this revision from to [RegionInfo] Verify getRegionFor.
Meinersbur updated this object.
Meinersbur added a reviewer: grosser.
Meinersbur set the repository for this revision to rL LLVM.
Meinersbur added a subscriber: llvm-commits.
grosser accepted this revision.Aug 9 2015, 8:53 AM
grosser edited edge metadata.

This patch looks good (besides some smaller comments below).

First, it seems this patch contains three different changes in one, which could probably be committed
separately.

  1. Use more descriptive error messages in RegionInfo's verifier
  1. Verify the BBMap during
  1. Take into account VerifyRegionInfo

We already have similar code in verifyRegion

template <class Tr>                                                              
void RegionBase<Tr>::verifyRegion() const {                                      
  // Only do verification when user wants to, otherwise this expensive check     
  // will be invoked by PMDataManager::verifyPreservedAnalysis when              
  // a regionpass (marked PreservedAll) finish.                                  
  if (!RegionInfoBase<Tr>::VerifyRegionInfo)                                     
    return;

I agree that the location you propose is better, but it probably makes sense to remove the old code and to briefly explain why this is safe and why you believe the new location is better.

include/llvm/Analysis/RegionInfo.h
695

Do not add verifyBBMap - at the beginning of the comment. This is an outdated style and LLVM is working towards removing such comments. If you
like you can drop (in a separate commit) the other 'functionName -' comment pieces as well in this file.

This revision is now accepted and ready to land.Aug 9 2015, 8:53 AM
  1. Use more descriptive error messages in RegionInfo's verifier

Would consider this trivial

  1. Take into account VerifyRegionInfo

We already have similar code in verifyRegion

template <class Tr>                                                              
void RegionBase<Tr>::verifyRegion() const {                                      
  // Only do verification when user wants to, otherwise this expensive check     
  // will be invoked by PMDataManager::verifyPreservedAnalysis when              
  // a regionpass (marked PreservedAll) finish.                                  
  if (!RegionInfoBase<Tr>::VerifyRegionInfo)                                     
    return;

I agree that the location you propose is better, but it probably makes sense to remove the old code and to briefly explain why this is safe and why you believe the new location is better.

Before this addition verifyAnalysis only called verifyRegionNest which would do nothing unless VerifyRegionInfo is true. To keep it that way, I added the check to verifyAnalysis.

However, I don't know whether verifyRegionNest is called from elsewhere so I left the check there to avoid behavioral change.

Meinersbur updated this revision to Diff 31669.Aug 10 2015, 8:02 AM
Meinersbur updated this object.
Meinersbur edited edge metadata.
Meinersbur marked an inline comment as done.

Adressed comments

Meinersbur updated this object.Aug 11 2015, 7:57 AM
This revision was automatically updated to reflect the committed changes.