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

[John McCall via Phabricator via cfe-commits]

rjmccall 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">;
+
----------------
These are the same now.


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


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2409
+        Src.getPointer(), ResultTy->getNumRows(), ResultTy->getNumColumns(),
+        Stride, "matrix");
+    return RValue::get(Result);
----------------
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.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:15178
+    Diag(StrideExpr->getBeginLoc(), diag::err_builtin_matrix_scalar_int_arg)
+        << 2 << 1;
+    ArgError = true;
----------------
fhahn wrote:
> rjmccall wrote:
> > It'd be nice to have comments for these magic values, like `/*stride*/ 2`.
> On second thought, the benefits of the magic numbers is rather small. I updated the diagnostics to take strings with the names of the arguments directly.
Actually, isn't this diagnostic redundant with the conversion you do in ApplyArgumentConversions?


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


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72781