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

[Florian Hahn via Phabricator via cfe-commits]

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


================
Comment at: clang/lib/Sema/SemaChecking.cpp:15107
+  {
+    ExprResult PtrConv = DefaultLvalueConversion(PtrExpr);
+    if (PtrConv.isInvalid())
----------------
rjmccall wrote:
> You should be doing `DefaultFunctionArrayLvalueConversion` here, which will eliminate all the special cases for arrays, both below and in IRGen.
> 
> It would've been fine to do that for your other builtin, too, it just wasn't necessary because it can never allow pointers.
Great thanks! That & together with `DependentTy` seems to solve the issue related to pointer type template expressions.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:15110
+      return PtrConv;
+    PtrExpr = PtrConv.get();
+  }
----------------
rjmccall wrote:
> Probably best to write it back into the call immediately at this point.
Updated to update the call immediately after conversions here and below.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:15116
+  if (auto *SubstTy = PtrTy->getAs<SubstTemplateTypeParmType>())
+    PtrTy = SubstTy->getReplacementType();
+
----------------
rjmccall wrote:
> Thinking that you need to do this is a huge indicator that you're doing something wrong later.  You should not have to remove  type sugar.
Not needed anymore, as mentioned above. Now the `remove_pointer` test also works :)


================
Comment at: clang/lib/Sema/SemaChecking.cpp:15121
+    Diag(PtrExpr->getBeginLoc(), diag::err_builtin_matrix_pointer_arg) << 0;
+    ArgError = true;
+  } else {
----------------
rjmccall wrote:
> You need to allow this expression to be dependently-typed.  There's a generic `DependentTy` that you can use as the result type of the call in this case.
I've updated the code to use `DependentTy` if any of the parts of the result matrix type is still dependently-typed.


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


================
Comment at: clang/lib/Sema/SemaChecking.cpp:15138
+  if (RowsExpr->isValueDependent() || RowsExpr->isTypeDependent() ||
+      ColumnsExpr->isValueDependent() || ColumnsExpr->isTypeDependent()) {
+    QualType ReturnType = Context.getDependentSizedMatrixType(
----------------
rjmccall wrote:
> Value dependence implies type dependence.  Butt you can't do these checks until after you've at least lowered placeholders.
> 
> It's not really necessary to build a DependentSizedMatrixType here rather than just using DependentTy.  It's not a bad thing to do — it *could* enable better type-checking of templates, like if you did this load and then had code trying to do a non-matrix operation on the result you could maybe reject that immediately instead of waiting for instantiation — but it's not really necessary, either.
> Value dependence implies type dependence. Butt you can't do these checks until after you've at least lowered placeholders.

I moved the conversion earlier.

> It's not really necessary to build a DependentSizedMatrixType here rather than just using DependentTy. It's not a bad thing to do — it *could* enable better type-checking of templates, like if you did this load and then had code trying to do a non-matrix operation on the result you could maybe reject that immediately instead of waiting for instantiation — but it's not really necessary, either.

Thanks, I opted to use `DependenTy`. `getDependentSizedMatrixType` asserts that the element type is a valid matrix element type (that's the reason for initially peeking through the template substitution expressions). We could still return a `DependentSizedMatrixType` if only the row or column expressions are dependently-typed, but from your comment I think it probably won't be worth it.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:15178
+    Diag(StrideExpr->getBeginLoc(), diag::err_builtin_matrix_scalar_int_arg)
+        << 2 << 1;
+    ArgError = true;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72781