[PATCH] D72281: [Matrix] Add matrix type to Clang.

[Florian Hahn via Phabricator via cfe-commits]

fhahn marked 4 inline comments as done.
fhahn added inline comments.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2109
+        if (!RowExprTemplateParam || !ColumnExprTemplateParam)
+          return Sema::TDK_Success;
+
----------------
rjmccall wrote:
> 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.
changed to llvm::function_ref


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2085
+
+      unsigned SubTDF = TDF & TDF_IgnoreQualifiers;
+
----------------
rjmccall wrote:
> 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.
Yeah, that's what I took originally. But it should be dropped, thanks!


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2122
+                  return Sema::TDK_NonDeducedMismatch;
+              }
+
----------------
rjmccall wrote:
> 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.
Got it, thanks! I've restructured (and simplified the code) as suggested. I've also added additional tests with row/column bounds as suggested (clang/test/CodeGenCXX/matrix-type.cpp starting at line 293 and negative ones clang/test/SemaCXX/matrix-type.cpp start at line 100)


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2143
+                return DeduceNonTypeTemplateArgument(S, TemplateParams, NTTP,
+                                                     DimExpr, Info, Deduced);
+            } else if (const ConstantMatrixType *ConstantMatrixArg =
----------------
rjmccall wrote:
> I'm curious why this extra check is necessary vs. just calling `DeduceNonTypeTemplateArgument` with `DimExpr` unconditionally.
The code can indeed be quite simplified if we can get a row/column expression for ConstantMatrixType as well. 

I've refactored the code to also keep the original row/column expression in ConstantMatrixType. The new code here is much more compact as the cost of increasing the size of ConstantMatrixType. I am not sure if the bigger size is a concern, but I would expect that it would not matter much in practice. 

If it is not a concern, I would further refactor the code and move the expressions to MatrixType (which would further simplify the lambdas here). The main difference between ConstantMatrixType and DependentSizedMatrixType would be that ConstantMatrixTpye would also store the dimensions as integers (for easier/faster access). What do you think?

Alternatively we could potentially construct a new ConstantExpr from the integer dimensions in the lambda. Or keep the more clunky accessor lambdas.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72281