rjmccall added a comment.
`SubstTemplateTypeParmType` is a "sugar" type node, like a `typedef`, and code
should generally be looking through it automatically by using `getAs` rather
than `isa` / `dyn_cast`.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:15107
+ {
+ ExprResult PtrConv = DefaultLvalueConversion(PtrExpr);
+ if (PtrConv.isInvalid())
----------------
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.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:15110
+ return PtrConv;
+ PtrExpr = PtrConv.get();
+ }
----------------
Probably best to write it back into the call immediately at this point.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:15116
+ if (auto *SubstTy = PtrTy->getAs<SubstTemplateTypeParmType>())
+ PtrTy = SubstTy->getReplacementType();
+
----------------
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.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:15121
+ Diag(PtrExpr->getBeginLoc(), diag::err_builtin_matrix_pointer_arg) << 0;
+ ArgError = true;
+ } else {
----------------
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.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:15128
+ else
+ llvm_unreachable("Pointer Expression must be a pointer or an array");
+
----------------
`getAs<PointerType>()` is the right way to do this. (You won't need
`getAsArrayType` if you decay arrays properly above.)
================
Comment at: clang/lib/Sema/SemaChecking.cpp:15130
+
+ ElementTy.removeLocalConst();
+ if (!ConstantMatrixType::isValidElementType(ElementTy)) {
----------------
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.)
================
Comment at: clang/lib/Sema/SemaChecking.cpp:15138
+ if (RowsExpr->isValueDependent() || RowsExpr->isTypeDependent() ||
+ ColumnsExpr->isValueDependent() || ColumnsExpr->isTypeDependent()) {
+ QualType ReturnType = Context.getDependentSizedMatrixType(
----------------
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.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:15178
+ Diag(StrideExpr->getBeginLoc(), diag::err_builtin_matrix_scalar_int_arg)
+ << 2 << 1;
+ ArgError = true;
----------------
It'd be nice to have comments for these magic values, like `/*stride*/ 2`.
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
