[PATCH] D71463: Implement VectorType conditional operator GNU extension.

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 13 inline comments as done.
erichkeane added a comment.

New patch coming as soon as my build finishes.



================
Comment at: clang/docs/LanguageExtensions.rst:480
 =                                yes     yes     yes     yes
-:?                               yes     --      --      --
+:?[*]_                              yes     --      yes     --
 sizeof                           yes     yes     yes     yes
----------------
aaron.ballman wrote:
> Do these columns line up in the actual source file? They don't appear to line up in the Phab viewer, but I don't know if that's an artifact of Phab or not.
They don't, I had misread the sphinx doc it seems and was doing footnotes incorrectly.  A coworker shared the live-sphinx viewer so I corrected this.  See next patch.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4301
+    llvm::Type *condType = ConvertType(condExpr->getType());
+    llvm::VectorType *vecTy = cast<llvm::VectorType>(condType);
+    llvm::Value *zeroVec = llvm::Constant::getNullValue(vecTy);
----------------
aaron.ballman wrote:
> `auto *`
> 
> Also, should these three variables have identifiers starting with an uppercase like `CondV` et al?
Probably, I just C&P'ed this code from the above group :/


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:5757
+  const QualType EltTy =
+      cast<VectorType>(CondTy.getCanonicalType().getTypePtr())
+          ->getElementType();
----------------
aaron.ballman wrote:
> Do you actually need the `getTypePtr()` bit, or will the `cast<>` suffice to trigger the cast through `Type*`?
TIL!


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:5793
+    Diag(QuestionLoc, diag::err_conditional_vector_operand_type)
+        << /*isExtVector*/ true << RHSType;
+    return {};
----------------
aaron.ballman wrote:
> Shouldn't this pass in the `LHSType` instead because that's what's being tested?
Yep!


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:5805
+    // If both are vector types, they must be the same type.
+    if (!Context.hasSameType(LHSType, RHSType)) {
+      Diag(QuestionLoc, diag::err_conditional_vector_mismatched_vectors)
----------------
aaron.ballman wrote:
> Do you need to canonicalize these types before comparing them?
No, both versions of this function canonicalize for me: https://clang.llvm.org/doxygen/ASTContext_8h_source.html#l02305


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

https://reviews.llvm.org/D71463