fhahn marked 12 inline comments as done. fhahn added inline comments.
================ Comment at: clang/lib/Sema/SemaChecking.cpp:15107 + { + ExprResult PtrConv = DefaultLvalueConversion(PtrExpr); + if (PtrConv.isInvalid()) ---------------- rjmccall wrote: > You should be doing `DefaultFunctionArrayLvalueConversion` here, which will > eliminate all the special cases for arrays, both below and in IRGen. > > It would've been fine to do that for your other builtin, too, it just wasn't > necessary because it can never allow pointers. Great thanks! That & together with `DependentTy` seems to solve the issue related to pointer type template expressions. ================ Comment at: clang/lib/Sema/SemaChecking.cpp:15110 + return PtrConv; + PtrExpr = PtrConv.get(); + } ---------------- rjmccall wrote: > Probably best to write it back into the call immediately at this point. Updated to update the call immediately after conversions here and below. ================ Comment at: clang/lib/Sema/SemaChecking.cpp:15116 + if (auto *SubstTy = PtrTy->getAs<SubstTemplateTypeParmType>()) + PtrTy = SubstTy->getReplacementType(); + ---------------- rjmccall wrote: > Thinking that you need to do this is a huge indicator that you're doing > something wrong later. You should not have to remove type sugar. Not needed anymore, as mentioned above. Now the `remove_pointer` test also works :) ================ Comment at: clang/lib/Sema/SemaChecking.cpp:15121 + Diag(PtrExpr->getBeginLoc(), diag::err_builtin_matrix_pointer_arg) << 0; + ArgError = true; + } else { ---------------- rjmccall wrote: > You need to allow this expression to be dependently-typed. There's a generic > `DependentTy` that you can use as the result type of the call in this case. I've updated the code to use `DependentTy` if any of the parts of the result matrix type is still dependently-typed. ================ Comment at: clang/lib/Sema/SemaChecking.cpp:15130 + + ElementTy.removeLocalConst(); + if (!ConstantMatrixType::isValidElementType(ElementTy)) { ---------------- rjmccall wrote: > It's almost never correct to do "local" qualifier manipulation in Sema. You > want to remove the `const` qualifier, which means removing it through however > much type sugar might be wrapping it. > > In reality, though, you actually want to remove *all* qualifiers, not just > `const`. So you should just use `getUnqualifiedType()`. (And then you need > to make sure that the IRGen code works on arbitrarily-qualified pointers. It > should already handle address spaces unless you're doing something very > strange. To handle `volatile`, you just need to be able to pass down a > `volatile` flag to your helper function. The other qualifiers should all > either not require special handling or not be allowed on integer/float types.) > In reality, though, you actually want to remove *all* qualifiers, not just > const. So you should just use getUnqualifiedType(). (And then you need to > make sure that the IRGen code works on arbitrarily-qualified pointers. It > should already handle address spaces unless you're doing something very > strange. To handle volatile, you just need to be able to pass down a volatile > flag to your helper function. The other qualifiers should all either not > require special handling or not be allowed on integer/float types.) Updated. Currently volatile cannot be specified for the `@llvm.matrix.columnwise.load/store` builtins. I'll put up an update for the intrinsics, for now I added an assertion in IRGen. I recently put up a patch that allows adding nuw/nsw info to the multiply builtin using operand bundles (D81166). For volatile, we could add another bundle or a boolean argument like we have for memcpy intrinsic I think. I am leaning towards an operand bundle version for this optional argument. Do you have any preference? ================ Comment at: clang/lib/Sema/SemaChecking.cpp:15138 + if (RowsExpr->isValueDependent() || RowsExpr->isTypeDependent() || + ColumnsExpr->isValueDependent() || ColumnsExpr->isTypeDependent()) { + QualType ReturnType = Context.getDependentSizedMatrixType( ---------------- rjmccall wrote: > Value dependence implies type dependence. Butt you can't do these checks > until after you've at least lowered placeholders. > > It's not really necessary to build a DependentSizedMatrixType here rather > than just using DependentTy. It's not a bad thing to do — it *could* enable > better type-checking of templates, like if you did this load and then had > code trying to do a non-matrix operation on the result you could maybe reject > that immediately instead of waiting for instantiation — but it's not really > necessary, either. > Value dependence implies type dependence. Butt you can't do these checks > until after you've at least lowered placeholders. I moved the conversion earlier. > It's not really necessary to build a DependentSizedMatrixType here rather > than just using DependentTy. It's not a bad thing to do — it *could* enable > better type-checking of templates, like if you did this load and then had > code trying to do a non-matrix operation on the result you could maybe reject > that immediately instead of waiting for instantiation — but it's not really > necessary, either. Thanks, I opted to use `DependenTy`. `getDependentSizedMatrixType` asserts that the element type is a valid matrix element type (that's the reason for initially peeking through the template substitution expressions). We could still return a `DependentSizedMatrixType` if only the row or column expressions are dependently-typed, but from your comment I think it probably won't be worth it. ================ Comment at: clang/lib/Sema/SemaChecking.cpp:15178 + Diag(StrideExpr->getBeginLoc(), diag::err_builtin_matrix_scalar_int_arg) + << 2 << 1; + ArgError = true; ---------------- rjmccall wrote: > It'd be nice to have comments for these magic values, like `/*stride*/ 2`. On second thought, the benefits of the magic numbers is rather small. I updated the diagnostics to take strings with the names of the arguments directly. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72781/new/ https://reviews.llvm.org/D72781 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits