[flang-commits] [flang] [OpenMP][Flang] Fix semantic check and scoping for declare mappers (PR #140560)

[Akash Banerjee via flang-commits]

TIFitis wrote:

> > One place I can think of placing the string is `llvm/include/llvm/Frontend/OpenMP/OMPConstants.h`, do you have any other suggestions?
> 
> Sounds OK to me. Alternative locations (if you omit parser changes) are in include/flang/Semantics/openmp*.h.
> 

I've created a PR for this here. For now the string is used in the parse tree as well so I've added it to OMPConstants.h, perhaps we can move it to include/flang/Semantics/openmp*.h in the future.

> > As for retaining std::optional, the reason to drop std::optional is that the name is optional in source but always carries a default value attached to it along with a symbol. With the name field being optional, we end up with a dangling default symbol which makes symbol lookups weird along with scoping rules.
> 
> A possibility is to change it from `std::optional<Name>` to `Name`.
> 
> > Secondly, I've changed it to string instead of Name, is that the name field doesn't have a data member to store non-static names, since it only has a char*. If we keep it as Name we will need other solutions in place to store the run-time generated string. The earlier iteration of this PR tried to do so but there is no clean way of doing it, and requires adding a new static storage.
> 
> We can add it to the Details in symbol.h like WithBindName, WithOmpDeclarative, OpenACCRoutineInfo etc.

I experimented with changing it to just `Name`, but the issue is that I couldn’t find a way to assign it a unique default name based on the derived type during parsing. Since `Name` is immutable, it can't be updated later. Given that the default mapper—and declare mappers in general—can be referenced both implicitly and explicitly, having a consistent naming scheme seems vital to me, especially for preserving scoping rules.

Otherwise, we’d have to account for multiple special cases during lowering, rather than relying on a single solution that works for both named and default mappers. Using a `string` feels like a clean approach to me, with the small trade-off of being inconsistent with other similar name fields. However, since the name field for a declare mapper is unique due to its default naming scheme, this seems like a worthwhile compromise.

That said, if you'd still prefer it to be changed to `Name`, I'm happy to revert this patch and let someone else take over, as I’m not too familiar with the parsing side of things.

Thanks! 🙂



https://github.com/llvm/llvm-project/pull/140560