Page MenuHomePhabricator

[ExtVector] Support ExtVectorType conditional operator
ClosedPublic

Authored by myhsu on May 26 2020, 9:50 AM.

Details

Summary

Extension vectors now can be used in element-wise conditional selector.
For example:

R[i] = C[i]? A[i] : B[i]

This feature was previously only enabled in OpenCL C. Now it's also
available in C. Not that it has different behaviors than GNU vectors
(i.e. vector_size). Extension vectors selects on signdness of the
vector. GNU vectors on the other hand do normal bool conversions. Also,
this feature is not available in C++.

Diff Detail

Event Timeline

myhsu created this revision.May 26 2020, 9:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2020, 9:51 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Anastasia added inline comments.May 28 2020, 7:44 AM
clang/docs/LanguageExtensions.rst
481

I think it was more clear as a footnote though.

489

has different behaviors depends on -> has different behaviors depending on

493

Maybe we can add a reference to OpenCL C section 6.3.9

clang/lib/Sema/SemaExpr.cpp
7616

Why do you need this change? I believe OpenCL makes the same restriction.

8104

Do you know where it is done for OpenCL? I think we should try to share the same code...

simoll added a subscriber: simoll.May 28 2020, 8:09 AM
myhsu marked 2 inline comments as done.May 28 2020, 1:38 PM
myhsu added inline comments.
clang/lib/Sema/SemaExpr.cpp
7616

I'm not quite sure what restriction you're referring here. If you're saying OpenCL only allow ext_vector_type on condition operand, I don't think that's the case since on line 7956 ~ 7957 of Sema/SemaExpr.cpp :

if (getLangOpts().OpenCL && Cond.get()->getType()->isVectorType())
    return OpenCLCheckVectorConditional(*this, Cond, LHS, RHS, QuestionLoc);

isVectorType() will be true for both GNU vector and ext_vector_type.

Regarding why i add the change here rather than merging with OpenCL's flow (which is related to another comments below), OpenCL's flow diverge into line 7957, as shown above, pretty early in the entire checking flow. And OpenCLCheckVectorConditional do other additional works. Most notably, promoting all scalar operands into vector if condition is a vector. I'm not sure if we should bring this feature to ext_vector

8104

It's inside OpenCLCheckVectorConditional function. As I mentioned in earlier comment, as long as we agree to bring every OpenCL features/rules regarding conditional vectors to ext_vector_type, I'm happy to reuse OpenCLCheckVectorConditional

myhsu updated this revision to Diff 267024.May 28 2020, 1:42 PM

Fix document section

myhsu updated this revision to Diff 267027.May 28 2020, 1:48 PM
myhsu marked 3 inline comments as done.
Anastasia added inline comments.May 29 2020, 8:34 AM
clang/lib/Sema/SemaExpr.cpp
7616

I'm not quite sure what restriction you're referring here.

You are restricting the use non-integer vectors in condition, is it not?

I'm not sure if we should bring this feature to ext_vector.

So what is it that you are trying to implement? My initial understanding was that you are just enabling behavior from OpenCL vectors to work in non-OpenCL mode. But you need to deviate from it?

8104

I don't know... I have no preference tbh. I think following OpenCL rules might be simpler from implementation and documentation perspective. But is it what you need?

myhsu marked 2 inline comments as done.May 29 2020, 9:41 AM
myhsu added inline comments.
clang/lib/Sema/SemaExpr.cpp
7616

My initial understanding was that you are just enabling behavior from OpenCL vectors to work in non-OpenCL mode

My initial thoughts was to bring, and only bring, OpenCL-style condition operands to ext_vector. Since that's the key to enable some hardware accelerations like the Intel AVX example I put in the original proposal.

But maybe I'm too cautious about introducing other OpenCL vector properties to ext_vector. After crafting some test cases, I found that there is little to no impact on simply using full set of OpenCL vector's semantic rules. So I'll update this patch to unify the checks between OpenCL vectors and ext_vector.

myhsu updated this revision to Diff 267277.May 29 2020, 10:02 AM

Use full set of OpenCL semantic rules for ext_vector_type

Anastasia accepted this revision.Jun 1 2020, 12:45 PM

LGTM! Thanks! Please address small documentation nitpick before committing...

Btw does your change fix this bug:
https://bugs.llvm.org/show_bug.cgi?id=33103

clang/docs/LanguageExtensions.rst
496

s6.3.i -> s6.3.9

This revision is now accepted and ready to land.Jun 1 2020, 12:45 PM
myhsu marked an inline comment as done.Jun 2 2020, 9:01 AM
myhsu added a comment.Jun 2 2020, 9:49 AM

Btw does your change fix this bug:
https://bugs.llvm.org/show_bug.cgi?id=33103

As mentioned in the description, this patch only affect C code not C++ code. So strictly speaking this patch doesn't fix the bug, but if the same code attached in the bug report is compiled as C code it will pass.

This revision was automatically updated to reflect the committed changes.