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

Reply via email to