[PATCH] D25866: [Sema] Support implicit scalar to vector conversions

[Simon Dardis via Phabricator via cfe-commits]

sdardis added inline comments.


================
Comment at: lib/Sema/SemaExpr.cpp:8051
+  if (!LHSVecType) {
+    assert(RHSVecType && "RHSVecType is not a vector!");
     if (!tryVectorConvertAndSplat(*this, (IsCompAssign ? nullptr : &LHS),
----------------
bruno wrote:
> `tryVectorConvertAndSplat` does more than plain scalar splat; it supports a more general type of CK_IntegralCast, see the comment on one of your changes to the tests below.
> 
> I suggest that instead of reusing this function, you should create another one that only handles the cases we actually want to support for non-ext vectors (i.e. for GCC compat). 
(ignore this, phabricator's web interface is acting up)


================
Comment at: lib/Sema/SemaExpr.cpp:8267
+  }
+
   // Otherwise, use the generic diagnostic.
----------------
bruno wrote:
> sdardis wrote:
> > bruno wrote:
> > > This change seems orthogonal to this patch. Can you make it a separated patch with the changes from test/Sema/vector-cast.c?
> > This change is a necessary part of this patch and it can't be split out in sensible fashion.
> > 
> > For example, line 329 of test/Sema/zvector.c adds a scalar signed character to a vector of signed characters. With just this change we would report "cannot convert between scalar type 'signed char' and vector type '__vector signed char' (vector of 16 'signed char' values) as implicit conversion would cause truncation".
> > 
> > This is heavily misleading for C and C++ code as we don't perform implicit conversions and signed char could be implicitly converted to a vector of signed char without the loss of precision.
> > 
> > To make this diagnostic precise without performing implicit conversions requires determining cases where implicit conversion would cause truncation and reporting that failure reason, or defaulting to the generic diagnostic.
> > 
> > Keeping this change as part of this patch is cleaner and simpler as it covers both implicit conversions and more precise diagnostics.
> Can you double check whether we should support these GCC semantics for getLangOpts().ZVector? If not, then this should be introduced in a separate patch/commit.
See my comment above.


================
Comment at: lib/Sema/SemaExpr.cpp:8171
+        return LHSType;
+    }
   }
----------------
bruno wrote:
> It's not clear to me whether clang should support the GCC semantics when in `getLangOpts().AltiVec || getLangOpts().ZVector`, do you?
> 
> You can remove the curly braces for the `if` and `else` here.
We should, GCC performs scalar to vector conversions regardless of what cpu features are available. If the target machine doesn't have vector support, GCC silently scalarizes the operation.

https://gcc.gnu.org/onlinedocs/gcc-6.2.0/gcc/Vector-Extensions.html#Vector-Extensions


https://reviews.llvm.org/D25866