This is an archive of the discontinued LLVM Phabricator instance.

Fix an error after D21678
ClosedPublic

Authored by vbyakovlcl on Sep 12 2016, 12:19 PM.

Details

Summary

This fixes an error and gcc incompatibilities in vector shifts reported by Akira Hatanaka


From: Akira Hatanaka <ahatanak@gmail.com>
Date: Fri, Sep 2, 2016 at 3:00 AM
Subject: Re: [PATCH] D21678: Fix For pr28288 - Error message in shift of vector values
To: vladimir.1@gmail.com, ulrich.weigand@de.ibm.com, amjad.aboud@intel.com, guy.benyei@intel.com, aaron.ballman@gmail.com
Cc: ahatanak@gmail.com, andreybokhanko@gmail.com, anastasia.stulova@arm.com, dmitry.polukhin@gmail.com, cfe-commits@lists.llvm.org

ahatanak added a subscriber: ahatanak.
ahatanak added a comment.

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?

Repository:

rL LLVM

https://reviews.llvm.org/D21678

Diff Detail

Event Timeline

vbyakovlcl retitled this revision from to Fix an error after D21678.
vbyakovlcl updated this object.
vbyakovlcl added reviewers: ahatanak, aaron.ballman.
vbyakovlcl set the repository for this revision to rL LLVM.
vbyakovlcl changed the visibility from "Public (No Login Required)" to "All Users".
ahatanak added inline comments.Sep 12 2016, 3:00 PM
llvm/tools/clang/lib/Sema/SemaExpr.cpp
8733

It wasn't clear to me why we have to call DefaultFunctionArrayLvalueConversion instead of UsualUnaryConversions. Is it possible to come up with a test case that would fail without this change, and if it is, can you add it to vecshift.c?

8746

We no longer error out when LHS is not a vector, so it should mention that either the LHS or the RHS might not be a vector.

8751

This comment should be updated since other vector types (e.g., gcc's vector type) are handled by this function too.

8773

Is it correct to always create an ExtVectorType here? What if the RHS is a GenericVector?

8786

Is it possible to split this out to another patch? The first patch would fix handling of (scalar << vector) and the second patch would make clang reject vector shifts if element sizes of the LHS and RHS are different. It's not clear whether it's correct to reject the latter case, so perhaps you can discuss it in a separate patch?

8790

When the LHS is a scalar, we check the type of the LHS and the element type of the RHS, and if necessary, cast the LHS to the element type of the RHS. What's the reason we don't do the same here?

vbyakovlcl updated this revision to Diff 71135.Sep 13 2016, 3:46 AM
vbyakovlcl removed rL LLVM as the repository for this revision.
vbyakovlcl changed the visibility from "All Users" to "Public (No Login Required)".
vbyakovlcl added inline comments.Sep 14 2016, 8:30 AM
llvm/tools/clang/lib/Sema/SemaExpr.cpp
8733

I cannot remember why I did this change. Removing it doesn't cause any new fails.

8746

I added a comment.

8786

Ok. I removed this and will fail a new review after committing of this review.

8790

No need, because CodeGen inserts a conversion.

define <8 x i16> @foo1(<8 x i16> %n, i32 %m) #0 {
entry:

%n.addr = alloca <8 x i16>, align 16
%m.addr = alloca i32, align 4
store <8 x i16> %n, <8 x i16>* %n.addr, align 16
store i32 %m, i32* %m.addr, align 4
%0 = load <8 x i16>, <8 x i16>* %n.addr, align 16
%1 = load i32, i32* %m.addr, align 4
%splat.splatinsert = insertelement <8 x i32> undef, i32 %1, i32 0
%splat.splat = shufflevector <8 x i32> %splat.splatinsert, <8 x i32> undef, <8 x i32> zeroinitializer
%sh_prom = trunc <8 x i32> %splat.splat to <8 x i16>
%shl = shl <8 x i16> %0, %sh_prom
ret <8 x i16> %shl

We need insert a conversion of scalar LHS to the RHS element type because we must return a vector of the RHS type.

ahatanak added inline comments.Sep 14 2016, 11:35 AM
llvm/tools/clang/lib/Sema/SemaExpr.cpp
8751

Should we mention that any vectors used as shift operands have to have integer element types? This comment gives the impression that only OpenCL vectors need to be integers.

8790

OK. So the rule is that the type of the scalar operand is normally converted to the vector element type of the other operand before being transformed to a vector, but we don't have to do this when it's used as the RHS (shift amount) because IRGen generates the correct IR without the conversion?

llvm/tools/clang/test/Sema/vecshift.c
56

I don't think this patch was necessary to have clang print diagnostic "vector operands do not have the same number of elements"? If that's the case, I think it's better to add these lines in a separate commit.

vbyakovlcl added inline comments.Sep 14 2016, 12:32 PM
llvm/tools/clang/lib/Sema/SemaExpr.cpp
8790

Yes. I think a CodeGen test should be added for checking of IR correctness.

llvm/tools/clang/test/Sema/vecshift.c
56

Do you propose to delete lines with this diagnostic?

ahatanak added inline comments.Sep 14 2016, 1:43 PM
llvm/tools/clang/test/Sema/vecshift.c
56

Yes, I think you can move them to a separate patch. Doing so will help people who read the patch understand what problems you are trying to fix with this patch.

vbyakovlcl updated this revision to Diff 71498.Sep 15 2016, 5:59 AM
ahatanak edited edge metadata.Sep 15 2016, 12:51 PM

LGTM with a nit in test case.

llvm/tools/clang/test/CodeGen/vecshift.c
43 ↗(On Diff #71498)

This test fails on my machine because MaxVectorAlign is 128 (16B) for darwin. Maybe you can remove the alignment checks or add a triple to the RUN line?

vbyakovlcl updated this revision to Diff 71545.Sep 15 2016, 1:20 PM
vbyakovlcl edited edge metadata.
vbyakovlcl added inline comments.
llvm/tools/clang/test/CodeGen/vecshift.c
43 ↗(On Diff #71498)

Done

This revision was automatically updated to reflect the committed changes.