fhahn marked 9 inline comments as done.
fhahn 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">;
+
----------------
rjmccall wrote:
> These are the same now.
Ah yes, it has been unused actually. Dropped.
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2403
+ Address Src = Address::invalid();
+ Src = EmitPointerWithAlignment(E->getArg(0));
+
----------------
rjmccall wrote:
> This can be simplified now.
Folded the 2 statements.
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2409
+ Src.getPointer(), ResultTy->getNumRows(), ResultTy->getNumColumns(),
+ Stride, "matrix");
+ return RValue::get(Result);
----------------
rjmccall wrote:
> 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.
Pass the alignment through to the builder. It sets the align attribute for the
pointer argument now
================
Comment at: clang/lib/Sema/SemaChecking.cpp:15130
+
+ ElementTy.removeLocalConst();
+ if (!ConstantMatrixType::isValidElementType(ElementTy)) {
----------------
rjmccall wrote:
> fhahn wrote:
> > 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?
> The only thing I really care about is that it can't be dropped implicitly.
> That's not legal with a bundle, but it's maybe a little more likely as an
> error of omission. On the other hand, you do need to pass down an alignment,
> and that really shouldn't be an optional argument, so maybe that's an
> opportunity to add a `volatile` argument as well.
I think for alignment we can use the align call attribute, which is what I am
using in the latest update.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:15094
+Sema::SemaBuiltinMatrixColumnMajorLoadOverload(CallExpr *TheCall,
+ ExprResult CallResult) {
+ if (checkArgCount(*this, TheCall, 4))
----------------
rjmccall wrote:
> You do need to check whether your extension is enabled in this builtin.
Done and also added a test.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:15112
+ TheCall->setArg(0, PtrExpr);
+ }
+
----------------
rjmccall wrote:
> You can just bail out early here (set a dependent type on the expression and
> return) if `PtrExpr` is type-dependent.
added early exit.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:15185
+ uint64_t Stride = Value.getZExtValue();
+ if (MaybeRows && Stride < *MaybeRows) {
+ Diag(StrideExpr->getBeginLoc(),
----------------
rjmccall wrote:
> Might as well hoist the `MaybeRows` check up so that we skip this whole thing
> if we don't have a row count.
Moved to the outer if.
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
