[flang-commits] [PATCH] D87906: [Flang][OpenACC] Fix for branching out issue in OpenACC parallel construct.

[sameeran joshi via Phabricator via flang-commits]

sameeranjoshi marked 3 inline comments as done.
sameeranjoshi added inline comments.


================
Comment at: flang/lib/Semantics/check-do-forall.cpp:920
+    if (StmtMatchesConstruct(
+            stmtName, stmtType, &(*constructName), construct)) {
       CheckDoConcurrentExit(stmtType, construct);
----------------
tskeith wrote:
> Can't `constructName` be nullopt?
I don't think we need to handle it here, because I see it's handled inside `StmtMatchesConstruct` at 
https://github.com/llvm/llvm-project/blob/551caec4a8af79483823e2940d40afb4c1df5da1/flang/lib/Semantics/check-do-forall.cpp#L918

I also tried to use the conventional approach of using a guard for std::optional.
```
if (constructName){
// Accessing the std::optional value by dereferencing it.

}
```
But I see regressions due to `CheckDoConcurrentExit`, which seem to have a dependency on the result from `StmtMatchesConstruct`.
Please correct me if I am wrong here.


================
Comment at: flang/lib/Semantics/tools.cpp:1337
+                    },
+      construct);
+}
----------------
tskeith wrote:
> sameeranjoshi wrote:
> > tskeith wrote:
> > > The call to `common::GetPtrFromOptional` could be moved outside of `std::visit` so that it doesn't have to be repeated in each lambda.
> > > 
> > > Even better might be to change the return type to `const <std::optional<parser::Name>> &`. Is there any reason to convert it to a pointer?
> > My knowledge of modern cpp seemed to pull me back.
> > I tried various different ways to return `const std::optional<parser::Name> &`, all resulting in either seg-fault or unknown regressions in fortran loops.
> > I have managed to do this change using a `return-by-value` and not `return-by-reference`.
> > If these changes still need to be modified, some suggestions on using a return-by-reference would be helpful.
> You may need to provide return types for the lambdas so that std::visit returns a reference and not a value. Like this:
> ```
>   return std::visit(
>       common::visitors{
>           [&](const parser::BlockConstruct *blockConstruct)
>               -> const std::optional<parser::Name> & {
>             return std::get<0>(blockConstruct->t).statement.v;
>           },
>           [&](const auto *a) -> const std::optional<parser::Name> & {
>             return std::get<0>(std::get<0>(a->t).statement.t);
>           },
>       },
>       construct);
> ```
Thank a lot!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87906/new/

https://reviews.llvm.org/D87906