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 at -1 {{candidate function [with T = float, R = 10]}}
>
> template <class T, unsigned long C>
> void use_matrix(matrix<T, 10, C> &m) {}
> // expected-note at -1 {{candidate function [with T = float, C = 10]}}
>
> void test_ambigous_deduction1() {
> matrix<float, 10, 10> m;
> use_matrix(m);
> // expected-error at -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