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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

`SubstTemplateTypeParmType` is a "sugar" type node, like a `typedef`, and code should generally be looking through it automatically by using `getAs` rather than `isa` / `dyn_cast`.



================
Comment at: clang/lib/Sema/SemaChecking.cpp:15107
+  {
+    ExprResult PtrConv = DefaultLvalueConversion(PtrExpr);
+    if (PtrConv.isInvalid())
----------------
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.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:15110
+      return PtrConv;
+    PtrExpr = PtrConv.get();
+  }
----------------
Probably best to write it back into the call immediately at this point.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:15116
+  if (auto *SubstTy = PtrTy->getAs<SubstTemplateTypeParmType>())
+    PtrTy = SubstTy->getReplacementType();
+
----------------
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.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:15121
+    Diag(PtrExpr->getBeginLoc(), diag::err_builtin_matrix_pointer_arg) << 0;
+    ArgError = true;
+  } else {
----------------
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.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:15128
+    else
+      llvm_unreachable("Pointer Expression must be a pointer or an array");
+
----------------
`getAs<PointerType>()` is the right way to do this.  (You won't need `getAsArrayType` if you decay arrays properly above.)


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


================
Comment at: clang/lib/Sema/SemaChecking.cpp:15138
+  if (RowsExpr->isValueDependent() || RowsExpr->isTypeDependent() ||
+      ColumnsExpr->isValueDependent() || ColumnsExpr->isTypeDependent()) {
+    QualType ReturnType = Context.getDependentSizedMatrixType(
----------------
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.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:15178
+    Diag(StrideExpr->getBeginLoc(), diag::err_builtin_matrix_scalar_int_arg)
+        << 2 << 1;
+    ArgError = true;
----------------
It'd be nice to have comments for these magic values, like `/*stride*/ 2`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72781