fhahn marked 3 inline comments as done.
fhahn added inline comments.
================
Comment at: clang/include/clang/Sema/Sema.h:12126
+ ExprResult SemaBuiltinMatrixColumnMajorLoadOverload(CallExpr *TheCall,
+ ExprResult CallResult);
----------------
rjmccall wrote:
> I don't think the word "overload" is doing anything in either of these method
> names.
Removed Overload here and for `SemaBuiltinMatrixTranspose`
================
Comment at: clang/lib/Sema/SemaChecking.cpp:15130
+
+ ElementTy.removeLocalConst();
+ if (!ConstantMatrixType::isValidElementType(ElementTy)) {
----------------
rjmccall wrote:
> 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.
I think we should be able to change them. I put up D81472 to update the
load/store intrinsics to update the name, types of stride/rows/columns and add
a `IsVolatile` flag. We could also pass the alignment as an extra parameter,
but it seems like the `align` attribute already provides a way to specify
alignment on a per-argument basis. Using it would mean we don't have to teach
various passes that use/propagate alignment info about the new intrinsics.
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
