[PATCH] D72778: [Matrix] Add __builtin_matrix_transpose to Clang.

[Florian Hahn via Phabricator via cfe-commits]

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


================
Comment at: clang/lib/Sema/SemaChecking.cpp:15051
+
+  Expr *Arg = TheCall->getArg(0);
+  if (!Arg->getType()->isConstantMatrixType()) {
----------------
rjmccall wrote:
> fhahn wrote:
> > rjmccall wrote:
> > > fhahn wrote:
> > > > rjmccall wrote:
> > > > > When a builtin has custom type-checking (`t`), you need to handle placeholders in the operands yourself, just like you would for an operator.
> > > > I added placeholder handling and added additional tests.
> > > Oh, I'm sorry, I gave you poor advice: you do need to handle placeholders, but more generally than that, you need an r-value.   Since you aren't doing any further conversions, you just need to call DefaultLValueConversion, which will also handle placeholders for you.
> > > 
> > > You will also need to store the checked argument back into the call, which you can only really test with an IRGen test.  This is one of the few places where we do actually directly mutate the AST.
> > Oh right, I wasn't sure about whether we need DefaultLValueConversion here or not. I tried to construct a C/C++ test case that would actually require it, but couldn't come up with a test. In any case, I updated it to use `DefaultLvalueConversion` instead and also adjust the argument before returning the updated call.
> IRGen is pretty lenient about missing LValueToRValue conversions because of the way it peepholes loads from complex l-value expressions.  But if you have basic `constexpr` stuff working in Sema, I think you can test this with lambdas.  A reference to a `constexpr` variable that's immediately loaded from is not an ODR-use, which means that within a lambda it doesn't result in a capture.  So a test like this should work, and it wouldn't if you didn't generate the LValueToRValue conversion in Sema.   (I think it doesn't actually test that the LValueToRValue conversions is actually added to the AST, though, just that you actually generated the AST node.)
> 
> ```
>   constexpr double4x4 m = {};
>   [] { return __builtin_matrix_transpose(m); }
> ```
Thanks for the example. Unfortunately constexpr for matrix types are not supported directly at the moment. Currently we get

```
matrix-type-builtins.cpp:97:26: error: constexpr variable cannot have non-literal type 'const double4x4_t' (aka 'double  __attribute__((matrix_type(4, 4)))const')
   constexpr double4x4_t m = {};
                         ^
```

Something like the code below works, but it requires an explicit capture

```
template <class T, unsigned R, unsigned C>
using matrix_type = T __attribute__((matrix_type(R, C)));

using double4x4_t = double __attribute__((matrix_type(4, 4)));
struct identmatrix_t {
  operator double4x4_t() const {
    double4x4_t result;
    for (unsigned i = 0; i != 4; ++i)
      result[i][i] = 1;
    return result;
  }
};

void test_conversion() {
   constexpr identmatrix_t m2;
 [&m2] { return __builtin_matrix_transpose((double4x4_t) m2); };
}
```

Given that, would it be OK to submit with the extra test commented out and a TODO to enable it once matrix types are supported as literal type in constexprs?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72778