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

[Florian Hahn via Phabricator via cfe-commits]

fhahn marked 9 inline comments as done.
fhahn added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10789
+def err_builtin_matrix_scalar_constant_unsigned_expr_arg: Error<
+  "%0 argument must be a constant unsigned integer expression">;
+
----------------
rjmccall wrote:
> These are the same now.
Ah yes, it has been unused actually. Dropped.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2403
+    Address Src = Address::invalid();
+    Src = EmitPointerWithAlignment(E->getArg(0));
+
----------------
rjmccall wrote:
> This can be simplified now.
Folded the 2 statements.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2409
+        Src.getPointer(), ResultTy->getNumRows(), ResultTy->getNumColumns(),
+        Stride, "matrix");
+    return RValue::get(Result);
----------------
rjmccall wrote:
> You should honor the alignment of `Src`.  If you emit a bunch of scattered loads, e.g. if the stride is not a constant, it might just be a cap on the alignment of the individual loads rather than a general optimization; but still, you should honor it.
Pass the alignment through to the builder. It sets the align attribute for the pointer argument now


================
Comment at: clang/lib/Sema/SemaChecking.cpp:15130
+
+    ElementTy.removeLocalConst();
+    if (!ConstantMatrixType::isValidElementType(ElementTy)) {
----------------
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.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:15094
+Sema::SemaBuiltinMatrixColumnMajorLoadOverload(CallExpr *TheCall,
+                                               ExprResult CallResult) {
+  if (checkArgCount(*this, TheCall, 4))
----------------
rjmccall wrote:
> You do need to check whether your extension is enabled in this builtin.
Done and also added a test.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:15112
+    TheCall->setArg(0, PtrExpr);
+  }
+
----------------
rjmccall wrote:
> You can just bail out early here (set a dependent type on the expression and return) if `PtrExpr` is type-dependent.
added early exit.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:15185
+      uint64_t Stride = Value.getZExtValue();
+      if (MaybeRows && Stride < *MaybeRows) {
+        Diag(StrideExpr->getBeginLoc(),
----------------
rjmccall wrote:
> Might as well hoist the `MaybeRows` check up so that we skip this whole thing if we don't have a row count.
Moved to the outer if.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72781/new/

https://reviews.llvm.org/D72781