Page MenuHomePhabricator

[Sema] Support Comma operator for fp16 vectors.
ClosedPublic

Authored by arames on Sep 24 2020, 3:22 PM.

Details

Summary

The current half vector was enforcing an assert expecting
"(LHS is half vector) == (RHS is half vector)"
for comma.

Diff Detail

Event Timeline

arames created this revision.Sep 24 2020, 3:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 24 2020, 3:22 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
arames requested review of this revision.Sep 24 2020, 3:22 PM
fhahn added a subscriber: fhahn.Sep 25 2020, 2:00 AM
fhahn added inline comments.
clang/lib/Sema/SemaExpr.cpp
13943–13946

I think you need braces around the expression before && "...", otherwise some compilers may complain.

clang/test/Sema/fp16vec-sema.c
29

nit: it might be slightly better to move it 2 lines further down, so all operators that assign the result are grouped together. Also, should we add 1, hv0 as well?

arames marked 2 inline comments as done.Sep 29 2020, 10:00 AM
arames added inline comments.
clang/test/Sema/fp16vec-sema.c
29

Moved.
I added 1, hv0 for completeness. It would hit the same path as the reverse.

arames updated this revision to Diff 295025.Sep 29 2020, 10:01 AM
arames marked an inline comment as done.

Addressed review comments.

ahatanak accepted this revision.Sep 29 2020, 1:46 PM
ahatanak added inline comments.
clang/test/Sema/fp16vec-sema.c
33

Since these diagnostics aren't relevant to the test, can you silence them by passing -Wno-unused-value or assigning the expressions to variables?

This revision is now accepted and ready to land.Sep 29 2020, 1:46 PM
arames updated this revision to Diff 295116.Sep 29 2020, 2:00 PM

Address review comments.

arames marked an inline comment as done.Sep 29 2020, 2:01 PM

I do not have commit rights, so it would be great if you can land it.

Thanks.

fhahn added a comment.Sep 30 2020, 1:22 AM

Could you adjust the commit message to be a bit more descriptive, e.g something like [Sema] Support Comma operator for fp16 vectors.

arames updated this revision to Diff 295316.Sep 30 2020, 9:09 AM

Update commit message.

Could you adjust the commit message to be a bit more descriptive, e.g something like [Sema] Support Comma operator for fp16 vectors.

Done!

fhahn added a comment.Sep 30 2020, 9:58 AM

Could you adjust the commit message to be a bit more descriptive, e.g something like [Sema] Support Comma operator for fp16 vectors.

Done!

Oh I think you'd need to edit the revision on Phabricator (top right on this page, edit revision). But if you are fine with it I can commit the patch for you with the adjusted commit title.

arames retitled this revision from Fix comma with half vectors. to [Sema] Support Comma operator for fp16 vectors..Sep 30 2020, 10:02 AM

Oh I think you'd need to edit the revision on Phabricator (top right on this page, edit revision). But if you are fine with it I can commit the patch for you with the adjusted commit title.

Done !
Thanks for your help !

fhahn accepted this revision.Sep 30 2020, 10:22 AM

LGTM, thanks. I'll commit this in a minute.

This revision was automatically updated to reflect the committed changes.