Respond to Review Comments & fix failing test

Author: Stylie777

flang/include/flang/Parser/parse-tree.h

@@ -267,6 +267,7 @@ struct AccEndCombinedDirective;
 struct OpenACCDeclarativeConstruct;
 struct OpenACCRoutineConstruct;
 struct OpenMPConstruct;
+struct OpenMPLoopConstruct;
 struct OpenMPDeclarativeConstruct;
 struct OmpEndLoopDirective;
 struct OmpMemoryOrderClause;
@@ -5021,13 +5022,13 @@ struct OpenMPBlockConstruct {
 };
 
 // OpenMP directives enclosing do loop
+using NestedConstruct =
+    std::variant<DoConstruct, common::Indirection<OpenMPLoopConstruct>>;
 struct OpenMPLoopConstruct {
   TUPLE_CLASS_BOILERPLATE(OpenMPLoopConstruct);
   OpenMPLoopConstruct(OmpBeginLoopDirective &&a)
       : t({std::move(a), std::nullopt, std::nullopt}) {}
-  std::tuple<OmpBeginLoopDirective,
-      std::optional<
-          std::variant<DoConstruct, common::Indirection<OpenMPLoopConstruct>>>,
+  std::tuple<OmpBeginLoopDirective, std::optional<NestedConstruct>,
       std::optional<OmpEndLoopDirective>>
       t;
 };

flang/lib/Lower/OpenMP/OpenMP.cpp

@@ -4231,7 +4231,8 @@ static void genOMP(lower::AbstractConverter &converter, lower::SymMap &symTable,
   mlir::Location currentLocation =
       converter.genLocation(beginLoopDirective.source);
 
-  auto &optLoopCons = std::get<1>(loopConstruct.t);
+  auto &optLoopCons =
+      std::get<std::optional<parser::NestedConstruct>>(loopConstruct.t);
   if (optLoopCons.has_value())
     if (auto *ompNestedLoopCons{
             std::get_if<common::Indirection<parser::OpenMPLoopConstruct>>(

flang/lib/Semantics/canonicalize-omp.cpp

@@ -166,8 +166,8 @@ class CanonicalizationOfOmp {
             std::get<parser::OmpBeginLoopDirective>(ompLoopCons->t);
         auto &beginLoopDirective =
             std::get<parser::OmpLoopDirective>(beginDirective.t);
-        if ((beginLoopDirective.v == llvm::omp::Directive::OMPD_unroll ||
-                beginLoopDirective.v == llvm::omp::Directive::OMPD_tile)) {
+        if (beginLoopDirective.v == llvm::omp::Directive::OMPD_unroll ||
+            beginLoopDirective.v == llvm::omp::Directive::OMPD_tile) {
           // iterate through the remaining block items to find the end directive
           // for the unroll/tile directive.
           parser::Block::iterator endIt;
@@ -188,18 +188,15 @@ class CanonicalizationOfOmp {
           }
           RewriteOpenMPLoopConstruct(*ompLoopCons, block, nextIt);
           auto &ompLoop =
-              std::get<std::optional<std::variant<parser::DoConstruct,
-                  common::Indirection<parser::OpenMPLoopConstruct>>>>(x.t);
-          ompLoop = std::optional<std::variant<parser::DoConstruct,
-              common::Indirection<parser::OpenMPLoopConstruct>>>{
-              std::variant<parser::DoConstruct,
-                  common::Indirection<parser::OpenMPLoopConstruct>>{
+              std::get<std::optional<parser::NestedConstruct>>(x.t);
+          ompLoop = std::optional<parser::NestedConstruct>{
+              parser::NestedConstruct{
                   common::Indirection{std::move(*ompLoopCons)}}};
           nextIt = block.erase(nextIt);
         } else {
-          messages_.Say(dir.source,
-              "Only OpenMP Loop Transformation Constructs can be nested within OpenMPLoopConstruct's"_err_en_US,
-              parser::ToUpperCaseLetters(dir.source.ToString()));
+          messages_.Say(beginLoopDirective.source,
+              "Only Loop Transformation Constructs or Loop Nests can be nested within Loop Constructs"_err_en_US,
+              parser::ToUpperCaseLetters(beginLoopDirective.source.ToString()));
         }
       } else {
         missingDoConstruct(dir);

flang/lib/Semantics/check-omp-structure.cpp

@@ -762,7 +762,7 @@ void OmpStructureChecker::Enter(const parser::OpenMPLoopConstruct &x) {
   }
   SetLoopInfo(x);
 
-  auto &optLoopCons = std::get<1>(x.t);
+  auto &optLoopCons = std::get<std::optional<parser::NestedConstruct>>(x.t);
   if (optLoopCons.has_value()) {
     if (const auto &doConstruct{
             std::get_if<parser::DoConstruct>(&*optLoopCons)}) {
@@ -789,7 +789,7 @@ const parser::Name OmpStructureChecker::GetLoopIndex(
   return std::get<Bounds>(x->GetLoopControl()->u).name.thing;
 }
 void OmpStructureChecker::SetLoopInfo(const parser::OpenMPLoopConstruct &x) {
-  auto &optLoopCons = std::get<1>(x.t);
+  auto &optLoopCons = std::get<std::optional<parser::NestedConstruct>>(x.t);
   if (optLoopCons.has_value()) {
     if (const auto &loopConstruct{
             std::get_if<parser::DoConstruct>(&*optLoopCons)}) {
@@ -863,7 +863,7 @@ void OmpStructureChecker::CheckIteratorModifier(const parser::OmpIterator &x) {
 
 void OmpStructureChecker::CheckLoopItrVariableIsInt(
     const parser::OpenMPLoopConstruct &x) {
-  auto &optLoopCons = std::get<1>(x.t);
+  auto &optLoopCons = std::get<std::optional<parser::NestedConstruct>>(x.t);
   if (optLoopCons.has_value()) {
     if (const auto &loopConstruct{
             std::get_if<parser::DoConstruct>(&*optLoopCons)}) {
@@ -1086,7 +1086,7 @@ void OmpStructureChecker::CheckDistLinear(
 
     // Match the loop index variables with the collected symbols from linear
     // clauses.
-    auto &optLoopCons = std::get<1>(x.t);
+    auto &optLoopCons = std::get<std::optional<parser::NestedConstruct>>(x.t);
     if (optLoopCons.has_value()) {
       if (const auto &loopConstruct{
               std::get_if<parser::DoConstruct>(&*optLoopCons)}) {

flang/lib/Semantics/resolve-directives.cpp

@@ -1796,7 +1796,7 @@ bool OmpAttributeVisitor::Pre(const parser::OpenMPLoopConstruct &x) {
   SetContextAssociatedLoopLevel(GetAssociatedLoopLevelFromClauses(clauseList));
 
   if (beginDir.v == llvm::omp::Directive::OMPD_do) {
-    auto &optLoopCons = std::get<1>(x.t);
+    auto &optLoopCons = std::get<std::optional<parser::NestedConstruct>>(x.t);
     if (optLoopCons.has_value()) {
       if (const auto &doConstruct{
               std::get_if<parser::DoConstruct>(&*optLoopCons)}) {
@@ -1965,7 +1965,7 @@ void OmpAttributeVisitor::PrivatizeAssociatedLoopIndexAndCheckLoopLevel(
   bool hasCollapseClause{
       clause ? (clause->Id() == llvm::omp::OMPC_collapse) : false};
 
-  auto &optLoopCons = std::get<1>(x.t);
+  auto &optLoopCons = std::get<std::optional<parser::NestedConstruct>>(x.t);
   if (optLoopCons.has_value()) {
     if (const auto &outer{std::get_if<parser::DoConstruct>(&*optLoopCons)}) {
       for (const parser::DoConstruct *loop{&*outer}; loop && level > 0;

flang/test/Semantics/OpenMP/loop-transformation-construct01.f90

@@ -34,13 +34,23 @@ subroutine loop_transformation_construct2
   integer :: v(i)
 
   !$omp do
-  !ERROR: Only OpenMP Loop Transformation Constructs can be nested within OpenMPLoopConstruct's
+  !ERROR: Only Loop Transformation Constructs or Loop Nests can be nested within Loop Constructs
   !$omp parallel do
   do x = 1, i
     v(x) = x(x) * 2
   end do
-  !! This error occurs because the `parallel do` end directive never gets matched.
-  !ERROR: The END PARALLEL DO directive must follow the DO loop associated with the loop construct
-  !$omp end parallel do
-  !$omp end do
+end subroutine
+
+subroutine loop_transformation_construct3
+  implicit none
+  integer :: i = 5
+  integer :: y
+  integer :: v(i)
+
+  !$omp do
+  do x = 1, i
+    v(x) = x(x) * 2
+  end do
+  !ERROR: A DO loop must follow the TILE directive
+  !$omp tile
 end subroutine