[PATCH] D72781: [Matrix] Add __builtin_matrix_column_load to Clang.

[John McCall via Phabricator via llvm-commits]

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