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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
