This is an archive of the discontinued LLVM Phabricator instance.

[Flang] Fix nested block constructs for SELECT CASE
ClosedPublic

Authored by cseo on May 22 2023, 9:38 AM.

Details

Summary

In some scenarios, a SELECT CASE could cause an error while lowering to FIR.
This was caused by a spurious extra branch added after the end statement. A
new test was added so this doesn't regress in the future.

Fixes #62726

Diff Detail

Event Timeline

cseo created this revision.May 22 2023, 9:38 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 22 2023, 9:38 AM
cseo requested review of this revision.May 22 2023, 9:38 AM
luporl added a subscriber: luporl.May 22 2023, 11:33 AM
luporl added inline comments.
flang/lib/Lower/Bridge.cpp
3851

Does checking for blockIsUnterminated() here work too, instead of using getIf()?
I'm not familiar with this code, but if the issue is being caused by successor->parentConstruct->lowerAsUnstructured() adding a terminator to the current block, then maybe checking for block termination may also catch other potential issues.

flang/test/Lower/select-case-statement.f90
431

CHECK is missing a colon.

Maybe the branches to bb7 could also be checked, for more context?

cseo added inline comments.May 22 2023, 11:47 AM
flang/lib/Lower/Bridge.cpp
3851

This check is already being done in the topmost if.

flang/test/Lower/select-case-statement.f90
431

Ack.

luporl added inline comments.May 22 2023, 12:56 PM
flang/lib/Lower/Bridge.cpp
3851

I thought lowerAsUnstructured() would try to lower the parentConstruct, but now I see it's not the case, so nevermind.

cseo updated this revision to Diff 524468.May 22 2023, 1:44 PM
cseo edited the summary of this revision. (Show Details)

Added extra checks to select-case-statement.f90

Fixed the missing colon and added extra checks for 'end select'.

Thanks @cseo for looking at this bug.

Most construct END statements do not have any associated code. That changed fairly recently to support finalization and deallocation code where needed. That includes deallocation of a character SELECT CASE trim(str) selector expression result, as here.

End-of-construct processing could be associated with an END construct statement (an EndSelectStmt here), or with the end of the construct itself (a CaseConstruct here). The two views must agree on which does what. Some other constructs already do this correctly. As with other cases, I recommend fixing the problem in construct code, rather than generic genFIR code. Something like the following, where the second set of diffs is for the genFIR SelectTypeConstruct function, which is also at risk for the same problem.

(It may be worth noting that SELECT statements are always unstructured in the FIR sense.)

diff --git a/flang/lib/Lower/Bridge.cpp b/flang/lib/Lower/Bridge.cpp
index c1dbac108b88..f1c93ccf5df8 100644
--- a/flang/lib/Lower/Bridge.cpp
+++ b/flang/lib/Lower/Bridge.cpp
@@ -1823,12 +1823,17 @@ private:

   void genFIR(const Fortran::parser::CaseConstruct &) {
     Fortran::lower::pft::Evaluation &eval = getEval();
     Fortran::lower::StatementContext stmtCtx;
     pushActiveConstruct(eval, stmtCtx);
-    for (Fortran::lower::pft::Evaluation &e : getEval().getNestedEvaluations())
-      genFIR(e);
+    for (Fortran::lower::pft::Evaluation &e :
+         getEval().getNestedEvaluations()) {
+      if (e.getIf<Fortran::parser::EndSelectStmt>())
+        maybeStartBlock(e.block);
+      else
+        genFIR(e);
+    }
     popActiveConstruct();
   }

   template <typename A>
   void genNestedStatement(const Fortran::parser::Statement<A> &stmt) {
@@ -2684,11 +2689,11 @@ private:
           }
         }
         builder->restoreInsertionPoint(crtInsPt);
         ++typeGuardIdx;
       } else if (eval.getIf<Fortran::parser::EndSelectStmt>()) {
-        genFIR(eval);
+        maybeStartBlock(eval.block);
         if (hasLocalScope)
           localSymbols.popScope();
       } else {
         genFIR(eval);
       }
cseo added a comment.May 22 2023, 7:25 PM

I recommend fixing the problem in construct code, rather than generic genFIR code. Something like the following, where the second set of diffs is for the genFIR SelectTypeConstruct function, which is also at risk for the same problem.

I see, makes sense. Thanks, I'll take a look and submit a new revision.

cseo updated this revision to Diff 524800.May 23 2023, 10:48 AM
cseo edited the summary of this revision. (Show Details)

Moved the fix from the generic genFIR code to SelectTypeConstruct/CaseConstruct

vdonaldson accepted this revision.May 23 2023, 11:14 AM

Looks good to me, thanks for addressing this!

flang/test/Lower/select-case-statement.f90
429

Exact ssa values such as %14 can be fragile, because ssa values could be deleted or inserted due to other arbitrary code changes. An unrelated mismatch can be avoided in this case by substituting %{{[0-9]+}} to match any valid ssa value here.

This revision is now accepted and ready to land.May 23 2023, 11:14 AM
cseo added inline comments.May 23 2023, 12:20 PM
flang/test/Lower/select-case-statement.f90
429

True. Forgot about this hardcoded value from my tests. Thanks for catching it.

cseo added a comment.May 24 2023, 6:31 AM

@vdonaldson btw, I don't have commit access. So, whenever you think this change is good to go, feel free to push it.

vdonaldson added a comment.EditedMay 24 2023, 9:06 AM

@vdonaldson btw, I don't have commit access. So, whenever you think this change is good to go, feel free to push it.

@cseo The preferred approach is to request commit access: obtaining-commit-access

(Don't forget to update the test details!)

cseo updated this revision to Diff 525270.May 24 2023, 10:59 AM
cseo marked an inline comment as not done.
cseo edited the summary of this revision. (Show Details)

Updated the testcase to remove a hardcoded SSA register variable.

cseo added a comment.May 24 2023, 10:59 AM

@vdonaldson btw, I don't have commit access. So, whenever you think this change is good to go, feel free to push it.

@cseo The preferred approach is to request commit access: obtaining-commit-access

(Don't forget to update the test details!)

OK, will do. I updated the test to remove the hardcoded register.

This revision was automatically updated to reflect the committed changes.
cseo marked an inline comment as done.