[PATCH 07/11] Fix keep-going mode in ScopDetection

[Tobias Grosser]

On 09/06/2014 02:42, Andreas Simbuerger wrote:
> After doing the top-down traversion in the ScopDetection (to find the
                            traversal
> canonical Scops) we do not expand the regions that were valid and had
> errors.
> As soon as we enter the expansion phase we will get a non canonical
> region and test the validity again. In this stage we do not check for
> errors and will always succeed in keep-going mode. This will not
> produce the same result as running without keep-going.

It is unclear if this describes the current state or the previous state.
(You sometimes use past tense 'were' and sometimes future 'will')

At best, explain in a first paragraph what this patch does and the state
the code is in after this patch. If necessary, add a second paragraph 
that explains the previous situation. Something like 'Before this patch
we .... This was bad as it caused different behavior with and without 
keep-going mode).

Cheers,
Tobias

> ---
>   lib/Analysis/ScopDetection.cpp | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/lib/Analysis/ScopDetection.cpp b/lib/Analysis/ScopDetection.cpp
> index 0907dd3..a15ca19 100644
> --- a/lib/Analysis/ScopDetection.cpp
> +++ b/lib/Analysis/ScopDetection.cpp
> @@ -546,13 +546,17 @@ Region *ScopDetection::expandRegion(Region &R) {
>     while (ExpandedRegion) {
>       DetectionContext Context(*ExpandedRegion, *AA, false /* verifying */);
>       DEBUG(dbgs() << "\t\tTrying " << ExpandedRegion->getNameStr() << "\n");
> +    // Only expand when we did not collect errors.
>
>       // Check the exit first (cheap)
> -    if (isValidExit(Context)) {
> +    if (isValidExit(Context) && !Context.Log.hasErrors()) {
>         // If the exit is valid check all blocks
>         //  - if true, a valid region was found => store it + keep expanding
>         //  - if false, .tbd. => stop  (should this really end the loop?)
> -      if (!allBlocksValid(Context))
> +      if (!allBlocksValid(Context) || Context.Log.hasErrors())
> +        break;
> +
> +      if (Context.Log.hasErrors())
>           break;
>
>         // Delete unnecessary regions (allocated by getExpandedRegion)
> @@ -628,10 +632,6 @@ void ScopDetection::findScops(Region &R) {
>     for (auto &SubRegion : R)
>       findScops(*SubRegion);
>
> -  // Do not expand when we had errors. Bad things may happen.
> -  if (IsValidRegion && HasErrors)
> -    return;
> -
>     // Try to expand regions.
>     //
>     // As the region tree normally only contains canonical regions, non canonical
> @@ -644,6 +644,11 @@ void ScopDetection::findScops(Region &R) {
>       ToExpand.push_back(SubRegion.get());
>
>     for (Region *CurrentRegion : ToExpand) {
> +    // Skip regions that had errors.
> +    bool HadErrors = Logs.hasErrors(CurrentRegion);
> +    if (HadErrors)
> +      continue;
> +
>       // Skip invalid regions. Regions may become invalid, if they are element of
>       // an already expanded region.
>       if (ValidRegions.find(CurrentRegion) == ValidRegions.end())
>