[PATCH] D11872: [RegionInfo] Verify getRegionFor

Tobias Grosser via llvm-commits llvm-commits at lists.llvm.org
Sun Aug 9 11:42:50 PDT 2015


On 08/09/2015 06:20 PM, Michael Kruse wrote:
> Meinersbur added a comment.
>
> In http://reviews.llvm.org/D11872#220006, @grosser wrote:
>
>> 1. Use more descriptive error messages in RegionInfo's verifier
>
>
> Would consider this trivial

Sure. Not highly important, but I like to keep such things separate as
to ensure they do not get lost if a patch needs to be reverted.

Feel free to commit such things just by yourself. They do not need
pre-commit review anyhow.
  
>> 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.
>
>
> 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.

OK, I think it is worth to add this information into the commit message.

Best,
Tobias


More information about the llvm-commits mailing list