[flang][msvc] Avoid ambiguous overload from base class. NFC.

[Meinersbur]

Summary

The method template <int KIND> Result operator()(const TypeParamInquiry<KIND> &) is defined in evaluate::GetShapeHelper as well as in its ancestor evaluate::Traverse. Normally, only the overloads of class that first defines method name would be considered, but the declaration using Base::operator() imports the base class methods with the same signature. Msvc interprets this as an ambiguity.

This pattern occurs in the following classes:

• evaluate::GetShapeHelper
• evaluate::IsConstantExprHelper
• evaluate::IsInitialDataTargetHelper
• evaluate::CheckSpecificationExprHelper

Fix by moving the generic TypeParamInquiry operator() overload into the derived classes that do not define this overload themselves.

This patch is part of the series to make flang compilable with MS Visual Studio.

[Meinersbur]

Considering more occurrences in check-expressions.cpp

[Meinersbur]

Bug report: https://developercommunity.visualstudio.com/content/problem/1159566/ambiguous-call-to-template-method-from-using-decla.html

[Meinersbur]

Fix missing helpers

[klausler]

The expression traversal framework in traverse.h depends on the C++17 compiler implementing base class overloads correctly; the traversal class implements a no-op visitor, and clients override the framework's no-op member functions for expression object types of interest. Please don't litter all of its clients with no-op overrides that will live forever.

[Meinersbur]

What is your alternative suggestion to fix the compiler error?

[klausler]

In D86410#2233859, @Meinersbur wrote:

What is your alternative suggestion to fix the compiler error?

Is the MSVC bug peculiar to template member functions like template <int K> operator()(const TypeParamInquiry<K> &) const? If so, is it peculiar to *just* TypeParamInquiry, or does it affect other templatized expression nodes as well? If that turns out to be the case, there's some changes to the expression representation that could be tried that would avoid the use of a template in that case.

Another possible work-around, if the problem is specific to this one expression representation class: specialize the member function in the general Traverse class for each of the integer kinds.

[Meinersbur]

In D86410#2233901, @klausler wrote:

Is the MSVC bug peculiar to template member functions like template <int K> operator()(const TypeParamInquiry<K> &) const? If so, is it peculiar to *just* TypeParamInquiry, or does it affect other templatized expression nodes as well?

The error happens with hidden methods with non-type templated method in templated base classes. The smallest reproducer I could create is this: https://godbolt.org/z/59ar5d. It does not occur when the template parameter is a class type: https://godbolt.org/z/1xo8jh. Thus, I think TypeParamInquiry is the only one affected.

If that turns out to be the case, there's some changes to the expression representation that could be tried that would avoid the use of a template in that case.

Could you elaborate?

Another possible work-around, if the problem is specific to this one expression representation class: specialize the member function in the general Traverse class for each of the integer kinds.

Which specializations for KIND do we need? 8,16,32,64, any other?

[klausler]

In D86410#2234030, @Meinersbur wrote:

In D86410#2233901, @klausler wrote:

Is the MSVC bug peculiar to template member functions like template <int K> operator()(const TypeParamInquiry<K> &) const? If so, is it peculiar to *just* TypeParamInquiry, or does it affect other templatized expression nodes as well?

The error happens with hidden methods with non-type templated method in templated base classes. The smallest reproducer I could create is this: https://godbolt.org/z/59ar5d. It does not occur when the template parameter is a class type: https://godbolt.org/z/1xo8jh. Thus, I think TypeParamInquiry is the only one affected.

If that turns out to be the case, there's some changes to the expression representation that could be tried that would avoid the use of a template in that case.

Could you elaborate?

Another possible work-around, if the problem is specific to this one expression representation class: specialize the member function in the general Traverse class for each of the integer kinds.

Which specializations for KIND do we need? 8,16,32,64, any other?

class TypeParameterInquiry could be redefined as an operation that always returned the same kind of INTEGER result, and explicit conversions to other kinds would be wrapped around its usage. It would become an operation allowed only in one instantiation of Expr<Type<TypeCategory::Integer, KIND>>. This change would be big enough to have some ripple effects, but I'd be happy to do it for you as an experiment that you could try with your MSVC environment.

The second work-around would use specializations for KIND values of 1, 2, 4, and 8.

[Meinersbur]

Use explicit method specializations in base class.

This solution is also accepted by msvc. A specialization TypeParamInquiry<16> is needed as well, otherwise it does not compiler.

[Meinersbur]

I am not familiar with TypeParameterInquiry. Since this solution works, a restructure might not be necessary unless you plan to do the change anyway.

[klausler]

In D86410#2234380, @Meinersbur wrote:

I am not familiar with TypeParameterInquiry. Since this solution works, a restructure might not be necessary unless you plan to do the change anyway.

I will post a patch here shortly that you can try.

[klausler]

tpi.patch12 KBDownload

Here's a patch that makes TypeParamInquiry monomorphic. Ignore the test failures with extra conversions -- I'll clean those up if you prefer this approach.

[Meinersbur]

I can confirm that shape.cpp compiles with your patch.

I'd prefer you patch, mostly because its reduced complexity. However, I think it's the code owner to decide which version they prefer.

[klausler]

In D86410#2234644, @Meinersbur wrote:

I can confirm that shape.cpp compiles with your patch.

I'd prefer you patch, mostly because its reduced complexity. However, I think it's the code owner to decide which version they prefer.

Thanks for checking. Let me know how it works out with the code owner.

[klausler]

I have my alternative patch coming soon to phabricator, and I'll include you on its review.

[Meinersbur]

According to CODE_OWNERS.TXT, that @sscalpone

[sscalpone]

@Meinersbur. Hi Michael, I believe you prefer @klausler to make the patch, so he's going ahead with it.

[klausler]

Please see https://reviews.llvm.org/D86551

[DavidTruby]

If this is now fixed in another way, can this review be closed?

[Meinersbur]

Yes, abandon in favor of D86551.

flang/include/flang/Evaluate/tools.h

if constexpr (common::HasMember<T, AllIntrinsicTypes> ||
           return known;
         }
       }
       return false;
     } else {
       return (*this)(x.u);
     }
   }
+  template <int KIND> Result operator()(const TypeParamInquiry<KIND> &x) const {
+    return (*this)(x.base());
+  }
 };
 
 template <typename A> bool IsVariable(const A &x) {
   if (auto known{IsVariableHelper{}(x)}) {
     return *known;
   } else {
     return false;
   }
if constexpr (common::HasMember<T, AllIntrinsicTypes> ||
           return known;
         }
       }
       return nullptr;
     } else {
       return (*this)(x.u);
     }
   }
+  template <int KIND> Result operator()(const TypeParamInquiry<KIND> &x) const {
+    return (*this)(x.base());
+  }
 };
 
 template <typename A> const Symbol *GetLastSymbol(const A &x) {
   if (auto known{GetLastSymbolHelper{}(x)}) {
     return *known;
   } else {
     return nullptr;
   }
struct GetSymbolVectorHelper
   Result Combine(Result &&a, Result &&b) {
     a.insert(a.end(), b.begin(), b.end());
     return std::move(a);
   }
   Result operator()(const Symbol &) const;
   Result operator()(const Component &) const;
   Result operator()(const ArrayRef &) const;
   Result operator()(const CoarrayRef &) const;
+  template <int KIND> Result operator()(const TypeParamInquiry<KIND> &x) const {
+    return (*this)(x.base());
+  }
 };
 template <typename A> SymbolVector GetSymbolVector(const A &x) {
   return GetSymbolVectorHelper{}(x);
 }
 
 // GetLastTarget() returns the rightmost symbol in an object designator's
 // SymbolVector that has the POINTER or TARGET attribute, or a null pointer
 // when none is found.

 // TODO: capture such scalar expansions in temporaries, flatten everything
 struct UnexpandabilityFindingVisitor
     : public AnyTraverse<UnexpandabilityFindingVisitor> {
   using Base = AnyTraverse<UnexpandabilityFindingVisitor>;
   using Base::operator();
   UnexpandabilityFindingVisitor() : Base{*this} {}
   template <typename T> bool operator()(const FunctionRef<T> &) { return true; }
   bool operator()(const CoarrayRef &) { return true; }
+  template <int KIND> bool operator()(const TypeParamInquiry<KIND> &x) const {
+    return (*this)(x.base());
+  }
 };
 
 template <typename T> bool IsExpandableScalar(const Expr<T> &expr) {
   return !UnexpandabilityFindingVisitor{}(expr);
 }
 
 } // namespace Fortran::evaluate
 

flang/include/flang/Evaluate/traverse.h

public:
   }
   Result operator()(const NamedEntity &x) const {
     if (const Component * component{x.UnwrapComponent()}) {
       return visitor_(*component);
     } else {
       return visitor_(x.GetFirstSymbol());
     }
   }
-  template <int KIND> Result operator()(const TypeParamInquiry<KIND> &x) const {
-    return visitor_(x.base());
-  }
   Result operator()(const Triplet &x) const {
     return Combine(x.lower(), x.upper(), x.stride());
   }
   Result operator()(const Subscript &x) const { return visitor_(x.u); }
   Result operator()(const ArrayRef &x) const {
     return Combine(x.base(), x.subscript());
   }
   Result operator()(const CoarrayRef &x) const {
public:
   Result Default() const { return default_; }
   static Result Combine(Result &&x, Result &&y) {
     if (x) {
       return std::move(x);
     } else {
       return std::move(y);
     }
   }
+  template <int KIND> Result operator()(const TypeParamInquiry<KIND> &x) const {
+    return (*this)(x.base());
+  }
 
 private:
   Result default_{};
 };
 
 template <typename Visitor, typename Set,
     typename Base = Traverse<Visitor, Set>>
 struct SetTraverse : public Base {

flang/lib/Evaluate/check-expression.cpp

if (auto chars{
             result.attrs.test(characteristics::FunctionResult::Attr::Pointer) &&
             result.attrs.test(
                 characteristics::FunctionResult::Attr::Contiguous);
       }
     }
     return false;
   }
 
+  template <int KIND> Result operator()(const TypeParamInquiry<KIND> &x) const {
+    return (*this)(x.base());
+  }
+
 private:
   // If the subscripts can possibly be on a simply-contiguous array reference,
   // return the rank.
   static std::optional<int> CheckSubscripts(
       const std::vector<Subscript> &subscript) {
     bool anyTriplet{false};
     int rank{0};
     for (auto j{subscript.size()}; j-- > 0;) {

flang/lib/Evaluate/shape.cpp

public:
   using Base = Traverse<GetLowerBoundHelper, ExtentExpr>;
   using Base::operator();
   GetLowerBoundHelper(FoldingContext &c, int d)
       : Base{*this}, context_{c}, dimension_{d} {}
   static ExtentExpr Default() { return ExtentExpr{1}; }
   static ExtentExpr Combine(Result &&, Result &&) { return Default(); }
   ExtentExpr operator()(const Symbol &);
   ExtentExpr operator()(const Component &);
+  template <int KIND> Result operator()(const TypeParamInquiry<KIND> &x) const {
+    return (*this)(x.base());
+  }
 
 private:
   FoldingContext &context_;
   int dimension_;
 };
 
 auto GetLowerBoundHelper::operator()(const Symbol &symbol0) -> Result {
   const Symbol &symbol{symbol0.GetUltimate()};

flang/lib/Evaluate/tools.cpp

struct IsNullPointerHelper : public AllTraverse<IsNullPointerHelper, false> {
   using Base::operator();
   bool operator()(const ProcedureRef &call) const {
     auto *intrinsic{call.proc().GetSpecificIntrinsic()};
     return intrinsic &&
         intrinsic->characteristics.value().attrs.test(
             characteristics::Procedure::Attr::NullPointer);
   }
   bool operator()(const NullPointer &) const { return true; }
+  template <int KIND> bool operator()(const TypeParamInquiry<KIND> &x) const {
+    return (*this)(x.base());
+  }
 };
 bool IsNullPointer(const Expr<SomeType> &expr) {
   return IsNullPointerHelper{}(expr);
 }
 
 // GetSymbolVector()
 auto GetSymbolVectorHelper::operator()(const Symbol &x) const -> Result {
   if (const auto *details{x.detailsIf<semantics::AssocEntityDetails>()}) {

 struct CollectSymbolsHelper
     : public SetTraverse<CollectSymbolsHelper, semantics::SymbolSet> {
   using Base = SetTraverse<CollectSymbolsHelper, semantics::SymbolSet>;
   CollectSymbolsHelper() : Base{*this} {}
   using Base::operator();
   semantics::SymbolSet operator()(const Symbol &symbol) const {
     return {symbol};
   }
+  template <int KIND>
+  semantics::SymbolSet operator()(const TypeParamInquiry<KIND> &x) const {
+    return (*this)(x.base());
+  }
 };
 template <typename A> semantics::SymbolSet CollectSymbols(const A &x) {
   return CollectSymbolsHelper{}(x);
 }
 template semantics::SymbolSet CollectSymbols(const Expr<SomeType> &);
 template semantics::SymbolSet CollectSymbols(const Expr<SomeInteger> &);
 template semantics::SymbolSet CollectSymbols(const Expr<SubscriptInteger> &);
 

flang/lib/Semantics/check-data.cpp

return std::visit(
         subscriptChecker(subs.u);
   }
   template <typename T>
   bool operator()(const evaluate::FunctionRef<T> &) const { // C875
     context_.Say(source_,
         "Data object variable must not be a function reference"_err_en_US);
     return false;
   }
+  template <int KIND>
+  bool operator()(const evaluate::TypeParamInquiry<KIND> &x) const {
+    return (*this)(x.base());
+  }
   void RestrictPointer() { isPointerAllowed_ = false; }
 
 private:
   bool CheckSubscriptExpr(
       const std::optional<evaluate::IndirectSubscriptIntegerExpr> &x) const {
     return !x || CheckSubscriptExpr(*x);
   }
   bool CheckSubscriptExpr(

flang/lib/Semantics/check-do-forall.cpp

: public evaluate::SetTraverse<CollectActualArgumentsHelper,
           ActualArgumentSet> {
   using Base = SetTraverse<CollectActualArgumentsHelper, ActualArgumentSet>;
   CollectActualArgumentsHelper() : Base{*this} {}
   using Base::operator();
   ActualArgumentSet operator()(const evaluate::ActualArgument &arg) const {
     return Combine(ActualArgumentSet{arg},
         CollectActualArgumentsHelper{}(arg.UnwrapExpr()));
   }
+  template <int KIND>
+  ActualArgumentSet operator()(
+      const evaluate::TypeParamInquiry<KIND> &x) const {
+    return (*this)(x.base());
+  }
 };
 
 template <typename A> ActualArgumentSet CollectActualArguments(const A &x) {
   return CollectActualArgumentsHelper{}(x);
 }
 
 template ActualArgumentSet CollectActualArguments(const SomeExpr &);