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

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added inline comments.


================
Comment at: lib/Sema/SemaExpr.cpp:8064
+      ScalarCast = CK_FloatingCast;
+    } else if (ScalarTy->isIntegralType(S.Context)) {
+      // Determine if the integer constant can be expressed as a floating point
----------------
sdardis wrote:
> bruno wrote:
> > I don't see why it's necessary to check for all specific cases where the scalar is a constant. For all the others scenarios it should be enough to get the right answer via `getIntegerTypeOrder` or `getFloatTypeOrder`. For this is specific condition, the `else` part for the `CstScalar` below should also handle the constant case, right? 
> > 
> > 
> If we have a scalar constant, it is necessary to examine it's actual value to see if it can be casted without truncation. The scalar constant's type is somewhat irrelevant. Consider:
> 
>    v4f32 f(v4f32 a) {
>      return a + (double)1.0;
>    }
> 
> In this case examining the order only works if the vector's order is greater than or equal to the scalar constant's order. Instead, if we ask whether the scalar constant can be represented as a constant scalar of the vector's element type, then we know if we can convert without the loss of precision.
> 
> For integers, the case is a little more contrived due to native integer width, but the question is the same. Can a scalar constant X be represented as a given floating point type. Consider:
> 
>    v4f32 f(v4f32 a) {
>      return a + (signed int)1;
>     }
> 
> This would rejected for platforms where a signed integer's width is greater than the precision of float if we examined it based purely on types and their sizes. Instead, if we convert the integer to the float point's type with APFloat and convert back to see if we have the same value, we can determine if the scalar constant can be safely converted without the loss of precision.
> 
> I based the logic examining the value of the constant scalar based on GCC's behaviour, which implicitly converts scalars regardless of their type if they can be represented in the same type of the vector's element type without the loss of precision. If the scalar cannot be evaluated to a constant at compile time, then the size in bits for the scalar's type determines if it can be converted safely.
Right. Can you split the scalarTy <-> vectorEltTy checking logic into separate functions; one for cst-scalar-int-to-vec-elt-int and another for scalar-int-to-vec-elt-float? 


================
Comment at: lib/Sema/SemaExpr.cpp:8267
+  }
+
   // Otherwise, use the generic diagnostic.
----------------
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.


================
Comment at: lib/Sema/SemaExpr.cpp:8020
+      if (Order < 0)
+        return true;
+    }
----------------
Please also early return `!CstScalar && Order < 0` like you did below.


================
Comment at: lib/Sema/SemaExpr.cpp:8064
+                                 !ScalarTy->hasSignedIntegerRepresentation());
+        bool ignored = false;
+        Float.convertToInteger(
----------------
`ignored` => `Ignored` 


================
Comment at: lib/Sema/SemaExpr.cpp:8171
+        return LHSType;
+    }
   }
----------------
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.


================
Comment at: lib/Sema/SemaExpr.cpp:8182
+        return RHSType;
+    }
   }
----------------
Also remove braces here.


https://reviews.llvm.org/D25866