[PATCH] D11872: [RegionInfo] Verify getRegionFor

[Tobias Grosser via llvm-commits]

grosser accepted this revision.
grosser added a comment.
This revision is now accepted and ready to land.

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

2. Verify the BBMap during

3. 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.


================
Comment at: include/llvm/Analysis/RegionInfo.h:695
@@ -691,1 +694,3 @@
+  void verifyBBMap(const RegionT *SR) const;
+
   // isCommonDomFrontier - Returns true if BB is in the dominance frontier of
----------------
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.


Repository:
  rL LLVM

http://reviews.llvm.org/D11872