Page MenuHomePhabricator

[SemaExpr] Support lax conversions in assignments with vector and scalars with same size
ClosedPublic

Authored by bruno on Jun 24 2016, 2:16 PM.

Details

Summary

Before r266366, clang used to support constructs like:

typedef __attribute__((vector_size(8))) double float64x1_t;
typedef __attribute__((vector_size(16))) double float64x2_t;
float64x1_t vget_low_f64(float64x2_t __p0);

double y = 3.0 + vget_low_f64(v);

But it would reject:

double y = vget_low_f64(v) + 3.0;

It also always rejected assignments:

double y = vget_low_f64(v);

This patch: (a) revivies the behavior of 3.0 + vget_low_f64(v) prior to
r266366, (b) add support for vget_low_f64(v) + 3.0 and (c) add support for
assignments.

These vector semantics have never really been tied up but it seems
odd that we used to support some binop froms but do not support
assignment. If we did support scalar for the purposes of arithmetic, we
should probably be able to reinterpret as scalar for the purposes of
assignment too.

Diff Detail

Event Timeline

bruno updated this revision to Diff 61838.Jun 24 2016, 2:16 PM
bruno retitled this revision from to [SemaExpr] Support lax conversions in assignments with vector and scalars with same size.
bruno updated this object.
bruno added reviewers: rnk, rsmith.
bruno added a subscriber: cfe-commits.
rnk edited edge metadata.Jun 29 2016, 10:11 AM

After writing r266366, we discovered that GCC accepts none of the code in that test case, so we should consider turning -flax-vector-conversions off by default.

test/Sema/vector-cast.c
68

Why should we allow this conversion? I don't see how <2 x float> and double should be convertible. I'm not sure why we allow f2 += d above, but I think of it as "widening" the type from scalar to vector.

Would it be OK for your if we tightened our lax vector conversion checks to just allow conversion from <1 x T> to T? The test case in the summary seems like pretty reasonable code, even if GCC rejects.

bruno added a comment.Jul 5 2016, 4:35 PM
In D21700#470103, @rnk wrote:

After writing r266366, we discovered that GCC accepts none of the code in that test case, so we should consider turning -flax-vector-conversions off by default.

Makes sense, specially given the non obvious semantics; users shouldn't rely on this behavior by default without the knowledge of what's actually going on.

test/Sema/vector-cast.c
68

I misread the testcase, <2 x float> and double shouldn't be convertible indeed. Thanks for the catch.

Allowing conversions from <1 x T> to T is enough for us, it also sounds more correct.

bruno updated this revision to Diff 62802.Jul 5 2016, 4:36 PM
bruno edited edge metadata.

Update patch after Reid's suggestions.

rnk accepted this revision.Jul 6 2016, 7:52 AM
rnk edited edge metadata.

lgtm Seems OK for now. :)

This revision is now accepted and ready to land.Jul 6 2016, 7:52 AM
bruno closed this revision.Jul 6 2016, 11:12 AM

Thanks Reid,

Committed r274646.