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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
