rjmccall added a comment. In D72281#2023111 <https://reviews.llvm.org/D72281#2023111>, @fhahn wrote:
> Ah right, thanks for clarifying. I think I managed to fix the remaining > problems. Previously the patch did not handle DependentSizedMatrixTypes with > non-dependent constant dimensions properly (e.g. a DependentSizedMatrixType > could have one dependent and one non-dependent dimension). That's a > difference to DependentSizedArrayType. Now the example works as expected > (I've etxended it a bit). Cases like the one below are rejected > > template <class T, unsigned long R, unsigned long C> > using matrix = T __attribute__((matrix_type(R, C))); > > template <class T, unsigned long R> > void use_matrix(matrix<T, R, 10> &m) {} > // expected-note@-1 {{candidate function [with T = float, R = 10]}} > > template <class T, unsigned long C> > void use_matrix(matrix<T, 10, C> &m) {} > // expected-note@-1 {{candidate function [with T = float, C = 10]}} > > void test_ambigous_deduction1() { > matrix<float, 10, 10> m; > use_matrix(m); > // expected-error@-1 {{call to 'use_matrix' is ambiguous}} > } Yeah, that looks right to reject. ================ Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2109 + if (!RowExprTemplateParam || !ColumnExprTemplateParam) + return Sema::TDK_Success; + ---------------- fhahn wrote: > rjmccall wrote: > > fhahn wrote: > > > rjmccall wrote: > > > > You should just do this right. If you can find a template parameter in > > > > the parameter's row/column expression, you have to try to deduce it, > > > > and short-circuit if that fails. Deduction is order-agnostic, so don't > > > > worry about that. > > > > > > > > Also, you need to handle DependentSizedMatrixTypes here in order to get > > > > function template partial ordering right. Pattern the code on how > > > > DependentSizedArrayTypes would handle it. > > > Thanks, I've re-structured the code along the lines of the code for > > > DependentSizedArrayType > > Could you extract a lambda helper function that does the common logic for > > the rows/columns values? It's probably cleanest to pass in a rows/cols > > flag instead of trying to abstract more than that. > > > > If you do that, you'll also fix the bug where you're using ColumnNTTP in > > the rows case. :) > I wanted to make sure that's the right direction before opening too much time > on refactoring :) The fact that we have to use different accessors makes > things a bit tricky I think, but I've added a lambda (which takes the > accessors as lambdas as well. > Please use `llvm::function_ref` here. Or you could even use a member function pointer, up to you. ================ Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2085 + + unsigned SubTDF = TDF & TDF_IgnoreQualifiers; + ---------------- Is this ignore-qualifiers thing copied from arrays? If so, it's probably array-specific; qualified array types are a bit odd in the language. ================ Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2122 + return Sema::TDK_NonDeducedMismatch; + } + ---------------- I think I see what you're trying to do here, but you're missing a check. When the parameter expression is instantiation-dependent but not directly a parameter reference (in the standard: "[a] non-type template argument or an array bound in which a subexpression references a template parameter"), this is a non-deduced context and shouldn't lead to deduction failure. You should move the `getDeducedParameterFromExpr` call into this helper and then do the logic like this: - If the parameter expression is not instantiation-dependent, then return success if the argument expression is non-dependent and evaluates to the same constant, otherwise return a mismatch. - If the parameter expression is not a deducible parameter, then return success because this is a non-deduced context. - Otherwise do the rest of the logic below. Test case for this is something like `N + 1` as a row/column bound. You can't deduce from that, but you can potentially deduce from other places in the type. ================ Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2143 + return DeduceNonTypeTemplateArgument(S, TemplateParams, NTTP, + DimExpr, Info, Deduced); + } else if (const ConstantMatrixType *ConstantMatrixArg = ---------------- I'm curious why this extra check is necessary vs. just calling `DeduceNonTypeTemplateArgument` with `DimExpr` unconditionally. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72281/new/ https://reviews.llvm.org/D72281 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits