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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits