[PATCH] D108832: [Builtins] Support ext_vector_type args for __builtin_fminf.

[Florian Hahn via Phabricator via cfe-commits]

fhahn added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:16544
+                                 E, AA_Passing))
+      return ExprResult(true);
+
----------------
erichkeane wrote:
> Doing this as ExprResult(true) is absurdly jarring.
Updated, thanks!


================
Comment at: clang/lib/Sema/SemaChecking.cpp:16554
+
+  if (auto *VecType = ElementType->getAs<ExtVectorType>())
+    ElementType = VecType->getElementType();
----------------
erichkeane wrote:
> I wonder if this should just deal in `VectorType`, which is the type that intends to emulate the GCC vector types.  My understanding is `ExtVectorType` is for the clang-extended vector types.
I think we should extend this to all vector types and matrix types. I only went with ext_vector_types to keep things simple initially to make sure the direction is acceptable first.



================
Comment at: clang/lib/Sema/SemaChecking.cpp:16575
+
+  // Update call argument to use the possibly converted matrix argument.
+  TheCall->setArg(0, A.get());
----------------
erichkeane wrote:
> What does this comment come from?  Is this supposed to be a 'FIXME'?  I don't see any other matrix stuff here.
Ah sorry, that was a left-over from where I get the boilerplate code to start with. It's removed in the latest version.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108832