fhahn marked an inline comment as done. fhahn added inline comments.
================ Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2143 + return DeduceNonTypeTemplateArgument(S, TemplateParams, NTTP, + DimExpr, Info, Deduced); + } else if (const ConstantMatrixType *ConstantMatrixArg = ---------------- 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? 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
