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

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2143
+                return DeduceNonTypeTemplateArgument(S, TemplateParams, NTTP,
+                                                     DimExpr, Info, Deduced);
+            } else if (const ConstantMatrixType *ConstantMatrixArg =
----------------
fhahn wrote:
> rjmccall wrote:
> > fhahn wrote:
> > > rjmccall wrote:
> > > > fhahn wrote:
> > > > > 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.
> > > > Eh, I'm torn about storing the expressions in `ConstantMatrixType`.  It's probably true that we wouldn't have a ton of these types and so the overall overhead might be negligible.  However, I think that if we choose to represent things this way, we probably ought to make "pristine" new `IntegerLiteral` expressions instead of remembering the original expressions, because we don't want weird syntactic quirks of the first matrix type we see to become embedded in the type forever.  We also run the risk of actually propagating those expressions around and getting bad diagnostics that point unexpectedly back at the first place that wrote a matrix type (or at the null location of a pristine literal) because of uniquing.  So I think it might be better to just continue to define away those problems by not storing expressions.
> > > Sounds good to me. Should I update the code here to use the separate lambdas again or would you prefer creating temporary expressions for the integer case?
> > I think I would prefer lambdas (or member pointers).
> Done, I've removed them again. The code with member pointers seems relatively nice, given the circumstances IMO
Thanks, this looks great.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72281