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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
