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

[Florian Hahn via Phabricator via cfe-commits]

fhahn marked 4 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:
> > > 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.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1918
+    switch (BuiltinID) {
+    case Builtin::BI__builtin_matrix_transpose:
+      return SemaBuiltinMatrixTransposeOverload(TheCall, TheCallResult);
----------------
rjmccall wrote:
> I didn't notice this before, but I think a single level of switch is fine; there's probably nothing common about matrix builtins that you're going to want to handle like this.
It's not required any more, after removing the `fenable_matrix` check. I dropped it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72778