rjmccall added inline comments.
================
Comment at: clang/include/clang/Sema/Sema.h:4708
+ /// conversion.
+ ExprResult tryConvertExprToTy(Expr *E, QualType Ty);
+
----------------
Please spell out "type" in the method name.
================
Comment at: clang/include/clang/Sema/Sema.h:12126
+ ExprResult SemaBuiltinMatrixColumnMajorLoadOverload(CallExpr *TheCall,
+ ExprResult CallResult);
----------------
I don't think the word "overload" is doing anything in either of these method
names.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:15130
+
+ ElementTy.removeLocalConst();
+ if (!ConstantMatrixType::isValidElementType(ElementTy)) {
----------------
fhahn wrote:
> 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.
Is there a reason this intrinsic can't be changed? You don't need to do it in
this patch, but using the "align" attribute as call-site attribute that's only
meaningful on certain initrinsics seems like a really poor representation
choice, especially for something as semantically important as an alignment
assumption.
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
