Page MenuHomePhabricator

Fix For pr28288 - Error message in shift of vector values
ClosedPublic

Authored by vbyakovl on Jun 24 2016, 3:08 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

vbyakovl retitled this revision from to Fix For pr28288 - Error message in shift of vector values.
vbyakovl updated this object.
vbyakovl added reviewers: uweigand, aaboud.
aaboud added a subscriber: cfe-commits.
vbyakovl added a subscriber: DmitryPolukhin.
Anastasia added inline comments.
llvm/tools/clang/lib/Sema/SemaExpr.cpp
8595 ↗(On Diff #62345)

an vector -> a vector

8675 ↗(On Diff #62345)

it seems like you don't need this statement as the next one is exactly the same!

Someone, please review this.

aaron.ballman added inline comments.Aug 4 2016, 5:31 AM
llvm/tools/clang/lib/Sema/SemaExpr.cpp
8597 ↗(On Diff #62464)

Why does this drop the mention of OpenCL in the function name?

8638 ↗(On Diff #62464)

It's good to keep language references in the comments, so I would leave the reference in even though this is being expanded for non-OpenCL vector types (unless the reference is incorrect).

8681–8683 ↗(On Diff #62464)

Why is it correct to remove semantic checking for vector operands?

llvm/tools/clang/test/Sema/shift.c
75 ↗(On Diff #62464)

Please clang-format the test case.

vbyakovl set the repository for this revision to rL LLVM.
vbyakovl added inline comments.Aug 8 2016, 4:56 AM
llvm/tools/clang/lib/Sema/SemaExpr.cpp
8597 ↗(On Diff #62464)

This routine has common applying rather than for OpenCL only.

8638 ↗(On Diff #62464)

I'll restore the language reference.

8681–8683 ↗(On Diff #62464)

This routine targets for checking operands of operations like plus, mul ..., but not shifts. The tests vect_shift_1 and vect_shift_2 (I added) are examples which were compiled with error.

llvm/tools/clang/test/Sema/shift.c
75 ↗(On Diff #62464)

Ok.

aaron.ballman added inline comments.Aug 8 2016, 7:46 AM
llvm/tools/clang/lib/Sema/SemaExpr.cpp
8681–8683 ↗(On Diff #67140)

Are you saying that calling CheckVectorOperands was always incorrect? Or that it's no longer required because it should be fully covered by checkVectorShift? Because the two methods do considerably different checking, and I would have expected there to be more behavioral differences in the tests by removing the call to CheckVectorOperands that suggests there are tests missing.

vbyakovl added inline comments.Aug 9 2016, 6:01 AM
llvm/tools/clang/lib/Sema/SemaExpr.cpp
8681–8683 ↗(On Diff #67140)

Yes, calling CheckVectorOperands is not correct here because it compares operand types to be the same. For shift it is not true.

aaron.ballman accepted this revision.Aug 9 2016, 6:17 AM
aaron.ballman edited edge metadata.

LGTM, but I am also not well-versed in vector operations, so you may want to wait a day or two for @uweigand or someone else to sign off as well.

llvm/tools/clang/lib/Sema/SemaExpr.cpp
8681–8683 ↗(On Diff #67140)

Thank you for clarifying!

This revision is now accepted and ready to land.Aug 9 2016, 6:17 AM

@uweigand Ulrich, any objections here? If not, I will commit this patch tomorrow (as the author, Vladimir, doesn't have commit access yet).

Andrey

This revision was automatically updated to reflect the committed changes.

This patch causes clang to error out on the following code, which used to compile fine:

$ cat f2.c

typedef __attribute__((__ext_vector_type__(8))) unsigned short vector_ushort8;

vector_ushort8 foo1(void) {
  return 1 << (vector_ushort8){7,6,5,4,3,2,1,0};
}

$ clang f2.c -c

clang used to transform the scaler operand to a vector operand (similar to the way gcc's vector is handled) when compiling for normal c/c++ (but printed an error message when compiling for opencl), but this patch dropped the check for LangOpts added in r230464 and changed that behavior. I don't think this was intentional?