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

[Florian Hahn via Phabricator via llvm-commits]

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