rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.
LGTM.
================
Comment at: clang/include/clang/Sema/Sema.h:12126
+ ExprResult SemaBuiltinMatrixColumnMajorLoadOverload(CallExpr *TheCall,
+ ExprResult CallResult);
----------------
fhahn wrote:
> rjmccall wrote:
> > I don't think the word "overload" is doing anything in either of these
> > method names.
> Removed Overload here and for `SemaBuiltinMatrixTranspose`
Thanks.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:15130
+
+ ElementTy.removeLocalConst();
+ if (!ConstantMatrixType::isValidElementType(ElementTy)) {
----------------
fhahn wrote:
> 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.
Ah, I'd forgotten that when we updated memcpy etc., we made it specify an
alignment with an alignment parameter attribute instead of a separate argument.
Yes, that's fine to imitate.
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
