rjmccall added inline comments.
================ 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: > > > > 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). 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
