[PATCH] D128750: [C++20] Implement P2113R0: Changes to the Partial Ordering of Constrained Functions

Mon Oct 17 13:02:33 PDT 2022

mizvekov added inline comments.

================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:5511-5539
+  template <typename T1, typename T2,
+            std::enable_if_t<std::is_same<T1, T2>::value, bool> = true>
+  bool operator()(T1 *PS1, T2 *PS2) {
+    return hasEqualTemplateArgumentList(
+        PS1->getTemplateArgsAsWritten()->arguments(),
+        PS2->getTemplateArgsAsWritten()->arguments());
+  }
----------------
ychen wrote:
> mizvekov wrote:
> > mizvekov wrote:
> > > mizvekov wrote:
> > > > mizvekov wrote:
> > > > > ychen wrote:
> > > > > > mizvekov wrote:
> > > > > > > I think you are not supposed to use the `TemplateArgsAsWritten` here.
> > > > > > > 
> > > > > > > The injected arguments are 'Converted' arguments, and the transformation above, by unpacking the arguments, is reversing just a tiny part of the conversion process.
> > > > > > > 
> > > > > > > It's not very meaningful to canonicalize the arguments as written to perform a semantic comparison, as that works well just for some kinds of template arguments, like types and templates, but not for other kinds in which the conversion process is not trivial.
> > > > > > > 
> > > > > > > For example, I think this may fail to compare the same integers written in different ways, like `2` vs `1 + 1`.
> > > > > > Indeed. It can happen only when comparing one partial specialization with another. I think the standard does not require an implementation to deal with this but we could use the best effort without much overhead. For `2` vs `1+1` or similar template arguments that are not dependent, we could assume the equivalence because they wouldn't be in the partial ordering stage if they're not equivalent. For more complicated cases like `J+2` vs `J+1+1` where J is NTTP, let's stop trying (match GCC) because the overhead is a little bit high. 
> > > > > But I think the 'TemplateArgs', which are the specialization arguments and are available through `getTemplateArgs()`, are the converted arguments you want here, ie the AsWritten arguments converted against the template.
> > > > > 
> > > > > I don't see why you can't just use that.
> > > > > 
> > > > > How about we change:
> > > > > ```
> > > > >   if (!TemplateArgumentListAreEqual(S.getASTContext())(P1, P2))
> > > > >     return nullptr;
> > > > > ```
> > > > > 
> > > > > Into:
> > > > > 
> > > > > ```
> > > > >   {
> > > > >     ArrayRef<TemplateArgument> Args1 = P1->getTemplateArgs().asArray(), Args2;
> > > > >     if constexpr (IsMoreSpecialThanPrimaryCheck)
> > > > >       Args2 = P2->getInjectedTemplateArgs();
> > > > >     else
> > > > >       Args2 = P2->getTemplateArgs().asArray();
> > > > > 
> > > > >     if (Args1.size() != Args2.size())
> > > > >       return nullptr;
> > > > > 
> > > > >     for (unsigned I = 0, E = Args1.size(); I < E; ++I) {
> > > > >       TemplateArgument Arg2 = Args2[I];
> > > > >       // Unlike the specialization arguments, the injected arguments are not
> > > > >       // always canonical.
> > > > >       if constexpr (IsMoreSpecialThanPrimaryCheck)
> > > > >         Arg2 = S.Context.getCanonicalTemplateArgument(Arg2);
> > > > > 
> > > > >       // We use profile, instead of structural comparison of the arguments,
> > > > >       // because canonicalization can't do the right thing for dependent
> > > > >       // expressions.
> > > > >       llvm::FoldingSetNodeID IDA, IDB;
> > > > >       Args1[I].Profile(IDA, S.Context);
> > > > >       Arg2.Profile(IDB, S.Context);
> > > > >       if (IDA != IDB)
> > > > >         return nullptr;
> > > > >     }
> > > > >   }
> > > > > ```
> > > > > 
> > > > > That should work, right?
> > > > Actually, you can even further simplify this.
> > > > 
> > > > You can't have two different specializations with the same specialization arguments. These arguments are used as the key to unique them anyway.
> > > > 
> > > > So simplify my above suggestion to:
> > > > ```
> > > >   if constexpr (IsMoreSpecialThanPrimaryCheck) {
> > > >     ArrayRef<TemplateArgument> Args1 = P1->getTemplateArgs().asArray(),
> > > >                                Args2 = P2->getInjectedTemplateArgs();
> > > >     if (Args1.size() != Args2.size())
> > > >       return nullptr;
> > > > 
> > > >     for (unsigned I = 0, E = Args1.size(); I < E; ++I) {
> > > >       // We use profile, instead of structural comparison of the arguments,
> > > >       // because canonicalization can't do the right thing for dependent
> > > >       // expressions.
> > > >       llvm::FoldingSetNodeID IDA, IDB;
> > > >       Args1[I].Profile(IDA, S.Context);
> > > >       // Unlike the specialization arguments, the injected arguments are not
> > > >       // always canonical.
> > > >       S.Context.getCanonicalTemplateArgument(Args2[I]).Profile(IDB, S.Context);
> > > >       if (IDA != IDB)
> > > >         return nullptr;
> > > >     }
> > > >   }
> > > > ```
> > > Yet another improvement here is that you can do this check once, when we create the specialization, and then simply store a `hasSameArgumentsAsPrimaryInjected` flag.
> > > 
> > > When we create the specialization, we already profile the specialization arguments anyway, so it's another computation you can avoid repeating.
> > > 
> > > Could even avoid repeating the profiling of the injected arguments if there was justified runtime overhead there, which I doubt, trading that off with increased memory use.
> > It doesn't even need to be a flag to store in each SpecializationDecl, because there can be only one specialization with the same arguments as the primary injected arguments, for obvious reasons :D
> > 
> > So probably just a pointer in the TemplateDecl...
> 'TemplateArgs' is indeed what I needed, I don't know how I missed that. There could be multiple partial specializations with the same arguments as the primary injected arguments and they differ by constraints (https://godbolt.org/z/onME5ehEE). I'm unsure if adding a `hasSameArgumentsAsPrimaryInjected` flag would save much since the `IsMoreSpecialThanPrimaryCheck` is only performed once. We could avoid maybe one profiling by introducing the `hasSameArgumentsAsPrimaryInjected` flag though. On the other hand, it doesn't make the implementation much harder either. 
> 
> I did some compile-time measurements, and the results seem neutral https://llvm-compile-time-tracker.com/index.php?config=NewPM-O0-g&stat=instructions&remote=yuanfang-chen.
> There could be multiple partial specializations with the same arguments as the primary injected arguments and they differ by constraints
Ah yeah, that is true.

I think not adding the flag looks fine, thanks for looking into it!

I am working tentatively on another patch where comparing template argument lists would become as cheap as comparing two pointers, and canonicalizing would be just one pointer indirection, so that issue would become moot anyway.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128750