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: > > > 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 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
