fhahn marked 4 inline comments as done. fhahn added inline comments.
================ Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2109 + if (!RowExprTemplateParam || !ColumnExprTemplateParam) + return Sema::TDK_Success; + ---------------- rjmccall wrote: > fhahn wrote: > > rjmccall wrote: > > > fhahn wrote: > > > > rjmccall wrote: > > > > > You should just do this right. If you can find a template parameter > > > > > in the parameter's row/column expression, you have to try to deduce > > > > > it, and short-circuit if that fails. Deduction is order-agnostic, so > > > > > don't worry about that. > > > > > > > > > > Also, you need to handle DependentSizedMatrixTypes here in order to > > > > > get function template partial ordering right. Pattern the code on > > > > > how DependentSizedArrayTypes would handle it. > > > > Thanks, I've re-structured the code along the lines of the code for > > > > DependentSizedArrayType > > > Could you extract a lambda helper function that does the common logic for > > > the rows/columns values? It's probably cleanest to pass in a rows/cols > > > flag instead of trying to abstract more than that. > > > > > > If you do that, you'll also fix the bug where you're using ColumnNTTP in > > > the rows case. :) > > I wanted to make sure that's the right direction before opening too much > > time on refactoring :) The fact that we have to use different accessors > > makes things a bit tricky I think, but I've added a lambda (which takes the > > accessors as lambdas as well. > > > Please use `llvm::function_ref` here. Or you could even use a member > function pointer, up to you. changed to llvm::function_ref ================ Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2085 + + unsigned SubTDF = TDF & TDF_IgnoreQualifiers; + ---------------- rjmccall wrote: > Is this ignore-qualifiers thing copied from arrays? If so, it's probably > array-specific; qualified array types are a bit odd in the language. Yeah, that's what I took originally. But it should be dropped, thanks! ================ Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2122 + return Sema::TDK_NonDeducedMismatch; + } + ---------------- rjmccall wrote: > I think I see what you're trying to do here, but you're missing a check. > When the parameter expression is instantiation-dependent but not directly a > parameter reference (in the standard: "[a] non-type template argument or an > array bound in which a subexpression references a template parameter"), this > is a non-deduced context and shouldn't lead to deduction failure. You should > move the `getDeducedParameterFromExpr` call into this helper and then do the > logic like this: > > - If the parameter expression is not instantiation-dependent, then return > success if the argument expression is non-dependent and evaluates to the same > constant, otherwise return a mismatch. > - If the parameter expression is not a deducible parameter, then return > success because this is a non-deduced context. > - Otherwise do the rest of the logic below. > > Test case for this is something like `N + 1` as a row/column bound. You > can't deduce from that, but you can potentially deduce from other places in > the type. Got it, thanks! I've restructured (and simplified the code) as suggested. I've also added additional tests with row/column bounds as suggested (clang/test/CodeGenCXX/matrix-type.cpp starting at line 293 and negative ones clang/test/SemaCXX/matrix-type.cpp start at line 100) ================ Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2143 + return DeduceNonTypeTemplateArgument(S, TemplateParams, NTTP, + DimExpr, Info, Deduced); + } else if (const ConstantMatrixType *ConstantMatrixArg = ---------------- 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. 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
