rjmccall added inline comments.
================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10789 +def err_builtin_matrix_scalar_constant_unsigned_expr_arg: Error< + "%0 argument must be a constant unsigned integer expression">; + ---------------- These are the same now. ================ Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2403 + Address Src = Address::invalid(); + Src = EmitPointerWithAlignment(E->getArg(0)); + ---------------- This can be simplified now. ================ Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2409 + Src.getPointer(), ResultTy->getNumRows(), ResultTy->getNumColumns(), + Stride, "matrix"); + return RValue::get(Result); ---------------- You should honor the alignment of `Src`. If you emit a bunch of scattered loads, e.g. if the stride is not a constant, it might just be a cap on the alignment of the individual loads rather than a general optimization; but still, you should honor it. ================ Comment at: clang/lib/Sema/SemaChecking.cpp:15178 + Diag(StrideExpr->getBeginLoc(), diag::err_builtin_matrix_scalar_int_arg) + << 2 << 1; + ArgError = true; ---------------- fhahn wrote: > 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. Actually, isn't this diagnostic redundant with the conversion you do in ApplyArgumentConversions? ================ Comment at: clang/lib/Sema/SemaChecking.cpp:15094 +Sema::SemaBuiltinMatrixColumnMajorLoadOverload(CallExpr *TheCall, + ExprResult CallResult) { + if (checkArgCount(*this, TheCall, 4)) ---------------- You do need to check whether your extension is enabled in this builtin. ================ Comment at: clang/lib/Sema/SemaChecking.cpp:15112 + TheCall->setArg(0, PtrExpr); + } + ---------------- You can just bail out early here (set a dependent type on the expression and return) if `PtrExpr` is type-dependent. ================ Comment at: clang/lib/Sema/SemaChecking.cpp:15185 + uint64_t Stride = Value.getZExtValue(); + if (MaybeRows && Stride < *MaybeRows) { + Diag(StrideExpr->getBeginLoc(), ---------------- Might as well hoist the `MaybeRows` check up so that we skip this whole thing if we don't have a row count. 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