[Flang] Changes to mangling code

[kiranchandramohan]

Summary

Call static functions using the class name (fir::NameUniquer).
Add function for mangling derivedTypes.

All the name mangling functions that are ultimately called are
tested in unittests/Optimizer/InternalNamesTest.cpp.

Original Author: Eric Schweitz

#upstreaming #NFC mostly

[dblaikie]

For future reference might be worth separating independent patches and giving a more specific/descriptive title. The three lines in the description seem like separate patch descriptions.

Though the description mentions that the functions are all tested - yet it looks like this patch adds a new function/overload without any callers, so how is it tested?

[kiranchandramohan]

In D99967#2687886, @dblaikie wrote:

Thanks @dblaikie for highlighting the issues in this patch.

For future reference might be worth separating independent patches and giving a more specific/descriptive title. The three lines in the description seem like separate patch descriptions.

OK I will separate them and improve the description in future.

Though the description mentions that the functions are all tested - yet it looks like this patch adds a new function/overload without any callers, so how is it tested?

Apologies for the misunderstanding here. What i meant is that the function that is ultimately called (fir::NameUniquer::doType) is already tested and this is a wrapper around that function. I will check today if I can add a unit
test for this function (assuming that is the recommendation here).

[dblaikie]

In D99967#2688047, @kiranchandramohan wrote:

In D99967#2687886, @dblaikie wrote:

Thanks @dblaikie for highlighting the issues in this patch.

For future reference might be worth separating independent patches and giving a more specific/descriptive title. The three lines in the description seem like separate patch descriptions.

OK I will separate them and improve the description in future.

Thanks! No worries.

Though the description mentions that the functions are all tested - yet it looks like this patch adds a new function/overload without any callers, so how is it tested?

Apologies for the misunderstanding here. What i meant is that the function that is ultimately called (fir::NameUniquer::doType) is already tested and this is a wrapper around that function. I will check today if I can add a unit
test for this function (assuming that is the recommendation here).

Hmm - well, then there's the question of: What's the purpose of the new function? We don't usually add new code without usage (& then test the new code via that usage). But there are some times when it's suitable to implement new code with nothing but a unit test for coverage - usually only if that code is intended for API usage by clients outside the LLVM project.

What's the purpose/intended usage of this new code?

[kiranchandramohan]

Though the description mentions that the functions are all tested - yet it looks like this patch adds a new function/overload without any callers, so how is it tested?

Apologies for the misunderstanding here. What i meant is that the function that is ultimately called (fir::NameUniquer::doType) is already tested and this is a wrapper around that function. I will check today if I can add a unit
test for this function (assuming that is the recommendation here).

Hmm - well, then there's the question of: What's the purpose of the new function? We don't usually add new code without usage (& then test the new code via that usage). But there are some times when it's suitable to implement new code with nothing but a unit test for coverage - usually only if that code is intended for API usage by clients outside the LLVM project.

What's the purpose/intended usage of this new code?

Names have to be unique at the FIR (MLIR IR for Flang) layer. So the usage of name mangling is when we lower from the parse-tree of Flang to FIR. The bridge code which does this lowering has not yet landed in llvm-project/flang. The approach for upstreaming this portion is to bring in all the support functions and classes and then introduce the bridge code and show the lowering and testing for each Fortran construct. (Note that at the FIR layer and lowering to LLVM IR the agreed approach is to upstream pass by pass).

[dblaikie]

In D99967#2688105, @kiranchandramohan wrote:

Though the description mentions that the functions are all tested - yet it looks like this patch adds a new function/overload without any callers, so how is it tested?

Apologies for the misunderstanding here. What i meant is that the function that is ultimately called (fir::NameUniquer::doType) is already tested and this is a wrapper around that function. I will check today if I can add a unit
test for this function (assuming that is the recommendation here).

Hmm - well, then there's the question of: What's the purpose of the new function? We don't usually add new code without usage (& then test the new code via that usage). But there are some times when it's suitable to implement new code with nothing but a unit test for coverage - usually only if that code is intended for API usage by clients outside the LLVM project.

What's the purpose/intended usage of this new code?

Names have to be unique at the FIR (MLIR IR for Flang) layer. So the usage of name mangling is when we lower from the parse-tree of Flang to FIR. The bridge code which does this lowering has not yet landed in llvm-project/flang. The approach for upstreaming this portion is to bring in all the support functions and classes and then introduce the bridge code and show the lowering and testing for each Fortran construct.

That presents a challenge for testing - usually we'd implement partial support for a feature more end-to-end (ie: make it "work" but without all the functionality) then as functionality is added incrementally, so is corresponding testing.

But adding lots of implementation code that's basically "dead" until the final patches wire it up makes it hard to tell which parts are tested - it'll need to be significantly unit tested & be a bit out of sync with the way the rest of LLVM is authored and tested.

(Note that at the FIR layer and lowering to LLVM IR the agreed approach is to upstream pass by pass).

I'm not especially familiar with MLIR or Flang to properly parse this statement - could you rephrase/add more detail?

[kiranchandramohan]

In D99967#2688121, @dblaikie wrote:

In D99967#2688105, @kiranchandramohan wrote:

Though the description mentions that the functions are all tested - yet it looks like this patch adds a new function/overload without any callers, so how is it tested?

Apologies for the misunderstanding here. What i meant is that the function that is ultimately called (fir::NameUniquer::doType) is already tested and this is a wrapper around that function. I will check today if I can add a unit
test for this function (assuming that is the recommendation here).

Hmm - well, then there's the question of: What's the purpose of the new function? We don't usually add new code without usage (& then test the new code via that usage). But there are some times when it's suitable to implement new code with nothing but a unit test for coverage - usually only if that code is intended for API usage by clients outside the LLVM project.

What's the purpose/intended usage of this new code?

Names have to be unique at the FIR (MLIR IR for Flang) layer. So the usage of name mangling is when we lower from the parse-tree of Flang to FIR. The bridge code which does this lowering has not yet landed in llvm-project/flang. The approach for upstreaming this portion is to bring in all the support functions and classes and then introduce the bridge code and show the lowering and testing for each Fortran construct.

That presents a challenge for testing - usually we'd implement partial support for a feature more end-to-end (ie: make it "work" but without all the functionality) then as functionality is added incrementally, so is corresponding testing.

But adding lots of implementation code that's basically "dead" until the final patches wire it up makes it hard to tell which parts are tested - it'll need to be significantly unit tested & be a bit out of sync with the way the rest of LLVM is authored and tested.

I am not disagreeing with the two points that you suggest. The partial end-to-end support was suggested but did not get the flang community approval. I believe the difficulty included finding small patches which implement end to end functionality that can be upstreamed. For e.g. when i tried to upstream the parse-tree to FIR lowering (https://reviews.llvm.org/D79731) of an empty subroutine in May last year it was a huge patch and there was not much support for it.

(Note that at the FIR layer and lowering to LLVM IR the agreed approach is to upstream pass by pass).

I'm not especially familiar with MLIR or Flang to properly parse this statement - could you rephrase/add more detail?

I was just highlighting the difference in approach of upstreaming the parse tree -> MLIR lowering and the MLIR -> MLIR transformations/MLIR -> LLVM IR lowering.
There is a lot of support code required for the former (parse tree -> MLIR) due to the big difference in representation of the parse-tree and MLIR. And all this support code is written from scratch.
In the case of MLIR transformations or lowering to LLVM IR a lot of support code already exists in llvm-project and hence for these cases we can upstream in the traditional way and it is the agreed approach for upstreaming. This would be like upstreaming the lowering of each MLIR operation or upstreaming each pass which does a transformation on MLIR.

[dblaikie]

In D99967#2688269, @kiranchandramohan wrote:

In D99967#2688121, @dblaikie wrote:

In D99967#2688105, @kiranchandramohan wrote:

Though the description mentions that the functions are all tested - yet it looks like this patch adds a new function/overload without any callers, so how is it tested?

Apologies for the misunderstanding here. What i meant is that the function that is ultimately called (fir::NameUniquer::doType) is already tested and this is a wrapper around that function. I will check today if I can add a unit
test for this function (assuming that is the recommendation here).

Hmm - well, then there's the question of: What's the purpose of the new function? We don't usually add new code without usage (& then test the new code via that usage). But there are some times when it's suitable to implement new code with nothing but a unit test for coverage - usually only if that code is intended for API usage by clients outside the LLVM project.

What's the purpose/intended usage of this new code?

Names have to be unique at the FIR (MLIR IR for Flang) layer. So the usage of name mangling is when we lower from the parse-tree of Flang to FIR. The bridge code which does this lowering has not yet landed in llvm-project/flang. The approach for upstreaming this portion is to bring in all the support functions and classes and then introduce the bridge code and show the lowering and testing for each Fortran construct.

That presents a challenge for testing - usually we'd implement partial support for a feature more end-to-end (ie: make it "work" but without all the functionality) then as functionality is added incrementally, so is corresponding testing.

But adding lots of implementation code that's basically "dead" until the final patches wire it up makes it hard to tell which parts are tested - it'll need to be significantly unit tested & be a bit out of sync with the way the rest of LLVM is authored and tested.

I am not disagreeing with the two points that you suggest. The partial end-to-end support was suggested but did not get the flang community approval.

Got any links to records of the discussion there? (perhaps this is an awkward case of having flang commits going to the LLVM-commits list, but the flang community conversations aren't happening in llvm-dev or other llvm areas - though good that it catches some community norm differences that we'd like to keep closer to the established LLVM conventions)

It might be worth bringing up this aspect of this kind of incremental development in that conversation, perhaps it changes the weighting of alternatives to some degree.

I believe the difficulty included finding small patches which implement end to end functionality that can be upstreamed. For e.g. when i tried to upstream the parse-tree to FIR lowering (https://reviews.llvm.org/D79731) of an empty subroutine in May last year it was a huge patch and there was not much support for it.

Agreed, that seems like a rather large patch - though I wonder if it is as small as it could be (for instance, at a glance - I can't seem to find a use of the AbstractConverter (it's only derived by one type and doesn't seem to be used via references/pointers to the base type - so maybe FirConverter could exist without the extra abstraction as a first pass/minimizing of this patch and the base class could be added in whatever later patch needs that abstraction)

But, yeah - so long as things are tested as they're added, one way or another, it's not deeply problematic - whatever way flang folks would like, just a minor suggestion that it might be worth considering if a more functionally incremental approach would be possible/beneficial.

(Note that at the FIR layer and lowering to LLVM IR the agreed approach is to upstream pass by pass).

I'm not especially familiar with MLIR or Flang to properly parse this statement - could you rephrase/add more detail?

I was just highlighting the difference in approach of upstreaming the parse tree -> MLIR lowering and the MLIR -> MLIR transformations/MLIR -> LLVM IR lowering.
There is a lot of support code required for the former (parse tree -> MLIR) due to the big difference in representation of the parse-tree and MLIR. And all this support code is written from scratch.
In the case of MLIR transformations or lowering to LLVM IR a lot of support code already exists in llvm-project and hence for these cases we can upstream in the traditional way and it is the agreed approach for upstreaming. This would be like upstreaming the lowering of each MLIR operation or upstreaming each pass which does a transformation on MLIR.

Ah, thanks!

[mehdi_amini]

This comment was removed by mehdi_amini.

[kiranchandramohan]

I am not disagreeing with the two points that you suggest. The partial end-to-end support was suggested but did not get the flang community approval.

Got any links to records of the discussion there? (perhaps this is an awkward case of having flang commits going to the LLVM-commits list, but the flang community conversations aren't happening in llvm-dev or other llvm areas - though good that it catches some community norm differences that we'd like to keep closer to the established LLVM conventions)

It might be worth bringing up this aspect of this kind of incremental development in that conversation, perhaps it changes the weighting of alternatives to some degree.

These discussions mostly happened during the flang technical/biweekly calls that we have.

I believe the difficulty included finding small patches which implement end to end functionality that can be upstreamed. For e.g. when i tried to upstream the parse-tree to FIR lowering (https://reviews.llvm.org/D79731) of an empty subroutine in May last year it was a huge patch and there was not much support for it.

Agreed, that seems like a rather large patch - though I wonder if it is as small as it could be (for instance, at a glance - I can't seem to find a use of the AbstractConverter (it's only derived by one type and doesn't seem to be used via references/pointers to the base type - so maybe FirConverter could exist without the extra abstraction as a first pass/minimizing of this patch and the base class could be added in whatever later patch needs that abstraction)

But, yeah - so long as things are tested as they're added, one way or another, it's not deeply problematic - whatever way flang folks would like, just a minor suggestion that it might be worth considering if a more functionally incremental approach would be possible/beneficial.

As I understand there is going to be some discussions going to happen in the next two weeks on trying to establish a better process for upstreaming the Flang code. May be there is enough support code upstream so that we can actually do the functionally incremental approach now. I will raise this during the discussions.

For this particular patch I will try to add some unittests.

flang/include/flang/Lower/Bridge.h

 #ifndef FORTRAN_LOWER_BRIDGE_H
 #define FORTRAN_LOWER_BRIDGE_H
 
 #include "flang/Common/Fortran.h"
 #include "flang/Lower/AbstractConverter.h"
 #include "flang/Optimizer/Support/KindMapping.h"
 #include "mlir/IR/BuiltinOps.h"
 
-namespace fir {
-struct NameUniquer;
-}
-
 namespace Fortran {
 namespace common {
 class IntrinsicTypeDefaultKinds;
 } // namespace common
 namespace evaluate {
 class IntrinsicProcTable;
 } // namespace evaluate
 namespace parser {
public:
   // Perform the creation of an mlir::ModuleOp
   //===--------------------------------------------------------------------===//
 
   /// Read in an MLIR input file rather than lowering Fortran sources.
   /// This is intended to be used for testing.
   void parseSourceFile(llvm::SourceMgr &);
 
   /// Cross the bridge from the Fortran parse-tree, etc. to MLIR dialects
-  void lower(const Fortran::parser::Program &program, fir::NameUniquer &uniquer,
+  void lower(const Fortran::parser::Program &program,
              const Fortran::semantics::SemanticsContext &semanticsContext);
 
 private:
   explicit LoweringBridge(
       const Fortran::common::IntrinsicTypeDefaultKinds &defaultKinds,
       const Fortran::evaluate::IntrinsicProcTable &intrinsics,
       const Fortran::parser::AllCookedSources &);
   LoweringBridge() = delete;

flang/include/flang/Lower/Mangler.h

 #ifndef FORTRAN_LOWER_MANGLER_H
 #define FORTRAN_LOWER_MANGLER_H
 
 #include "mlir/IR/BuiltinTypes.h"
 #include "llvm/ADT/StringRef.h"
 #include <string>
 
 namespace fir {
-struct NameUniquer;
 
 /// Returns a name suitable to define mlir functions for Fortran intrinsic
 /// Procedure. These names are guaranteed to not conflict with user defined
 /// procedures. This is needed to implement Fortran generic intrinsics as
 /// several mlir functions specialized for the argument types.
 /// The result is guaranteed to be distinct for different mlir::FunctionType
 /// arguments. The mangling pattern is:
 ///    fir.<generic name>.<result type>.<arg type>...
 /// e.g ACOS(COMPLEX(4)) is mangled as fir.acos.z4.z4
 std::string mangleIntrinsicProcedure(llvm::StringRef genericName,
                                      mlir::FunctionType);
 } // namespace fir
 
 namespace Fortran {
 namespace common {
 template <typename>
 class Reference;
 }
 
 namespace semantics {
 class Symbol;
-}
+class DerivedTypeSpec;
+} // namespace semantics
+
+namespace lower::mangle {
 
-namespace lower {
-namespace mangle {
+/// Convert a front-end Symbol to an internal name.
+/// If \p keepExternalInScope is true, the mangling of external symbols
+/// retains the scope of the symbol declaring externals. Otherwise,
+/// external symbols are mangled outside of any scope. Keeping the scope is
+/// useful in attributes where all the Fortran context is to be maintained.
+std::string mangleName(const semantics::Symbol &,
+                       bool keepExternalInScope = false);
 
-/// Convert a front-end Symbol to an internal name
-std::string mangleName(fir::NameUniquer &uniquer, const semantics::Symbol &);
+/// Convert a derived type instance to an internal name.
+std::string mangleName(const semantics::DerivedTypeSpec &);
 
+/// Recover the bare name of the original symbol from an internal name.
 std::string demangleName(llvm::StringRef name);
 
-} // namespace mangle
-} // namespace lower
+} // namespace lower::mangle
 } // namespace Fortran
 
 #endif // FORTRAN_LOWER_MANGLER_H

flang/include/flang/Optimizer/CodeGen/CodeGen.h

 //===-- Optimizer/CodeGen/CodeGen.h -- code generation ----------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef OPTIMIZER_CODEGEN_CODEGEN_H
 #define OPTIMIZER_CODEGEN_CODEGEN_H
 
 #include "mlir/IR/BuiltinOps.h"
 #include "mlir/Pass/Pass.h"
 #include "mlir/Pass/PassRegistry.h"
 #include <memory>
 
 namespace fir {
 
 struct NameUniquer;
 
 /// Prerequiste pass for code gen. Perform intermediate rewrites to perform
 /// the code gen (to LLVM-IR dialect) conversion.
 std::unique_ptr<mlir::Pass> createFirCodeGenRewritePass();
 
 /// Convert FIR to the LLVM IR dialect
-std::unique_ptr<mlir::Pass> createFIRToLLVMPass(NameUniquer &uniquer);
+std::unique_ptr<mlir::Pass> createFIRToLLVMPass();
 
 /// Convert the LLVM IR dialect to LLVM-IR proper
 std::unique_ptr<mlir::Pass>
 createLLVMDialectToLLVMPass(llvm::raw_ostream &output);
 
 // declarative passes
 #define GEN_PASS_REGISTRATION
 #include "flang/Optimizer/CodeGen/CGPasses.h.inc"
 
 } // namespace fir
 
 #endif // OPTIMIZER_CODEGEN_CODEGEN_H

flang/lib/Lower/Mangler.cpp

 //===-- Mangler.cpp -------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "flang/Lower/Mangler.h"
 #include "flang/Common/reference.h"
+#include "flang/Lower/Todo.h"
 #include "flang/Lower/Utils.h"
 #include "flang/Optimizer/Dialect/FIRType.h"
 #include "flang/Optimizer/Support/InternalNames.h"
 #include "flang/Semantics/tools.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
if (symbol.attrs().test(Fortran::semantics::Attr::MODULE) &&
     return iface;
   }
   return nullptr;
 }
 
 // Mangle the name of `symbol` to make it unique within FIR's symbol table using
 // the FIR name mangler, `mangler`
 std::string
-Fortran::lower::mangle::mangleName(fir::NameUniquer &uniquer,
-                                   const Fortran::semantics::Symbol &symbol) {
+Fortran::lower::mangle::mangleName(const Fortran::semantics::Symbol &symbol,
+                                   bool keepExternalInScope) {
   // Resolve host and module association before mangling
   const auto &ultimateSymbol = symbol.GetUltimate();
   auto symbolName = toStringRef(ultimateSymbol.name());
 
   return std::visit(
       Fortran::common::visitors{
           [&](const Fortran::semantics::MainProgramDetails &) {
-            return uniquer.doProgramEntry().str();
+            return fir::NameUniquer::doProgramEntry().str();
           },
           [&](const Fortran::semantics::SubprogramDetails &) {
             // Mangle external procedure without any scope prefix.
-            if (Fortran::semantics::IsExternal(ultimateSymbol))
-              return uniquer.doProcedure(llvm::None, llvm::None, symbolName);
+            if (!keepExternalInScope &&
+                Fortran::semantics::IsExternal(ultimateSymbol))
+              return fir::NameUniquer::doProcedure(llvm::None, llvm::None,
+                                                   symbolName);
             // Separate module subprograms must be mangled according to the
             // scope where they were declared (the symbol we have is the
             // definition).
             const auto *interface = &ultimateSymbol;
             if (const auto *mpIface = findInterfaceIfSeperateMP(ultimateSymbol))
               interface = mpIface;
             auto modNames = moduleNames(*interface);
-            return uniquer.doProcedure(modNames, hostName(*interface),
+            return fir::NameUniquer::doProcedure(modNames, hostName(*interface),
                                                  symbolName);
           },
           [&](const Fortran::semantics::ProcEntityDetails &) {
             // Mangle procedure pointers and dummy procedures as variables
             if (Fortran::semantics::IsPointer(ultimateSymbol) ||
                 Fortran::semantics::IsDummy(ultimateSymbol))
-              return uniquer.doVariable(moduleNames(ultimateSymbol),
-                                        hostName(ultimateSymbol), symbolName);
+              return fir::NameUniquer::doVariable(moduleNames(ultimateSymbol),
+                                                  hostName(ultimateSymbol),
+                                                  symbolName);
             // Otherwise, this is an external procedure, even if it does not
             // have an explicit EXTERNAL attribute. Mangle it without any
             // prefix.
-            return uniquer.doProcedure(llvm::None, llvm::None, symbolName);
+            return fir::NameUniquer::doProcedure(llvm::None, llvm::None,
+                                                 symbolName);
           },
           [&](const Fortran::semantics::ObjectEntityDetails &) {
             auto modNames = moduleNames(ultimateSymbol);
             auto optHost = hostName(ultimateSymbol);
             if (Fortran::semantics::IsNamedConstant(ultimateSymbol))
-              return uniquer.doConstant(modNames, optHost, symbolName);
-            return uniquer.doVariable(modNames, optHost, symbolName);
+              return fir::NameUniquer::doConstant(modNames, optHost,
+                                                  symbolName);
+            return fir::NameUniquer::doVariable(modNames, optHost, symbolName);
           },
-          [](const auto &) -> std::string {
-            assert(false);
-            return {};
+          [&](const Fortran::semantics::CommonBlockDetails &) {
+            return fir::NameUniquer::doCommonBlock(symbolName);
+          },
+          [&](const Fortran::semantics::DerivedTypeDetails &) -> std::string {
+            // Derived type mangling must used mangleName(DerivedTypeSpec&) so
+            // that kind type parameter values can be mangled.
+            llvm::report_fatal_error(
+                "only derived type instances can be mangled");
           },
+          [](const auto &) -> std::string { TODO_NOLOC("symbol mangling"); },
       },
       ultimateSymbol.details());
 }
 
+std::string Fortran::lower::mangle::mangleName(
+    const Fortran::semantics::DerivedTypeSpec &derivedType) {
+  // Resolve host and module association before mangling
+  const auto &ultimateSymbol = derivedType.typeSymbol().GetUltimate();
+  auto symbolName = toStringRef(ultimateSymbol.name());
+  auto modNames = moduleNames(ultimateSymbol);
+  auto optHost = hostName(ultimateSymbol);
+  llvm::SmallVector<std::int64_t> kinds;
+  for (const auto &param :
+       Fortran::semantics::OrderParameterDeclarations(ultimateSymbol)) {
+    const auto &paramDetails =
+        param->get<Fortran::semantics::TypeParamDetails>();
+    if (paramDetails.attr() == Fortran::common::TypeParamAttr::Kind) {
+      const auto *paramValue = derivedType.FindParameter(param->name());
+      assert(paramValue && "derived type kind parameter value not found");
+      auto paramExpr = paramValue->GetExplicit();
+      assert(paramExpr && "derived type kind param not explicit");
+      auto init = Fortran::evaluate::ToInt64(paramValue->GetExplicit());
+      assert(init && "derived type kind param is not constant");
+      kinds.emplace_back(*init);
+    }
+  }
+  return fir::NameUniquer::doType(modNames, optHost, symbolName, kinds);
+}
+
 std::string Fortran::lower::mangle::demangleName(llvm::StringRef name) {
   auto result = fir::NameUniquer::deconstruct(name);
   return result.second.name;
 }
 
 //===----------------------------------------------------------------------===//
 // Intrinsic Procedure Mangling
 //===----------------------------------------------------------------------===//