<html><head>
<meta http-equiv="content-type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#ffffff"><div><div style="font-family: Calibri,sans-serif; font-size: 11pt;">ok. LGTM then.<br><br>Pekka</div></div><div dir="ltr"><hr><span style="font-family: Calibri,sans-serif; font-size: 11pt; font-weight: bold;">Lähettäjä: </span><span style="font-family: Calibri,sans-serif; font-size: 11pt;"><a href="mailto:sameer.sahasrabuddhe@amd.com">Sameer Sahasrabuddhe</a></span><br><span style="font-family: Calibri,sans-serif; font-size: 11pt; font-weight: bold;">Lähetetty: </span><span style="font-family: Calibri,sans-serif; font-size: 11pt;">25.2.2015 7:11</span><br><span style="font-family: Calibri,sans-serif; font-size: 11pt; font-weight: bold;">Vastaanottaja: </span><span style="font-family: Calibri,sans-serif; font-size: 11pt;"><a href="mailto:pekka.jaaskelainen@tut.fi">Pekka Jääskeläinen</a>; <a href="mailto:j.ketema@imperial.ac.uk">Jeroen Ketema</a></span><br><span style="font-family: Calibri,sans-serif; font-size: 11pt; font-weight: bold;">Kopio: </span><span style="font-family: Calibri,sans-serif; font-size: 11pt;"><a href="mailto:cfe-commits@cs.uiuc.edu">llvm cfe</a>; <a href="mailto:tom@stellard.net">Tom Stellard</a></span><br><span style="font-family: Calibri,sans-serif; font-size: 11pt; font-weight: bold;">Aihe: </span><span style="font-family: Calibri,sans-serif; font-size: 11pt;">Re: r228382 - OpenCL: handle shift operator with vector operands</span><br><br></div>
Hello Pekka,<br>
<br>
I had a look at the current CodeGen tests ... they already cover
valid combinations of vector/scalar operands. The change improves
Sema to make sure that invalid combinations error out, and valid
combinations reach CodeGen. So in short, I can't think of any new
test to add to the CodeGen tests, and we should simply submit the
new change.<br>
<br>
Sameer.<br>
<br>
<div class="moz-cite-prefix">On 2/10/2015 10:13 PM, Pekka
Jääskeläinen wrote:<br>
</div>
<blockquote class=" cite" id="mid_54DA352F_5080508_tut_fi" cite="mid:54DA352F.5080508@tut.fi" type="cite">Hello,
<br>
<br>
I'm not knowledgeable enough on the Clang internals either (still
<br>
learning). Anyways, should this test that code is generated
<br>
correctly for <<=, not just that Sema doesn't complain about
it?
<br>
<br>
Other than this LGTM.
<br>
<br>
Pekka
<br>
<br>
On 02/10/2015 05:54 PM, Sahasrabuddhe, Sameer wrote:
<br>
<blockquote class=" cite" id="Cite_2564484" type="cite">Hello
Jeroen,
<br>
<br>
Thanks for the feedback. Now waiting for an LGTM from Pekka.
<br>
<br>
Sameer.
<br>
------------------------------------------------------------------------------
<br>
*From:* Jeroen Ketema [<a class="moz-txt-link-abbreviated" href="mailto:j.ketema@imperial.ac.uk">j.ketema@imperial.ac.uk</a>]
<br>
*Sent:* Tuesday, February 10, 2015 5:25 PM
<br>
*To:* Sahasrabuddhe, Sameer
<br>
*Cc:* llvm cfe; Tom Stellard; Pekka Jääskeläinen
<br>
*Subject:* Re: r228382 - OpenCL: handle shift operator with
vector operands
<br>
<br>
<br>
Hi,
<br>
<br>
I’m not knowledgeable enough to review the updated patch, but
all my test
<br>
cases now pass again.
<br>
<br>
Jeroen
<br>
<br>
<blockquote class=" cite" id="Cite_5199777" type="cite">On 09
Feb 2015, at 11:00, Sameer Sahasrabuddhe
<<a class="moz-txt-link-abbreviated" href="mailto:sameer.sahasrabuddhe@amd.com">sameer.sahasrabuddhe@amd.com</a>
<br>
<a class="moz-txt-link-rfc2396E" href="mailto:sameer.sahasrabuddhe@amd.com"><mailto:sameer.sahasrabuddhe@amd.com></a>> wrote:
<br>
<br>
Hi all,
<br>
<br>
Attached an updated patch that fixes the earlier patch for
vector shift
<br>
operators in OpenCL. Note the new argument "IsCompAssign" for
function
<br>
checkOpenCLVectorShift(), which was not handled in earlier
patch.
<br>
<br>
Sameer.
<br>
<br>
On 2/6/2015 11:02 PM, Tom Stellard wrote:
<br>
<blockquote class=" cite" id="Cite_7752365" type="cite">On
Fri, Feb 06, 2015 at 04:05:22PM +0000, Sahasrabuddhe, Sameer
wrote:
<br>
<blockquote class=" cite" id="Cite_3111629" type="cite">Hello
Jeroen,
<br>
<br>
Thanks for pointing this out! I will fix this as soon as
possible, but in the meantime, request you or anyone with
commit access to please revert my commit! I do not have
access to my work computer until Monday morning India
time. (Reminder to self: *never* commit on a Friday!).
<br>
</blockquote>
Hi,
<br>
<br>
I have reverted this. Can you add this test case when you
recommit.
<br>
<br>
Thanks,
<br>
Tom
<br>
<br>
<blockquote class=" cite" id="Cite_7696141" type="cite">Sameer.
<br>
________________________________________
<br>
From: Jeroen Ketema [<a class="moz-txt-link-abbreviated" href="mailto:j.ketema@imperial.ac.uk">j.ketema@imperial.ac.uk</a>]
<br>
Sent: Friday, February 06, 2015 9:15 PM
<br>
To: Sahasrabuddhe, Sameer
<br>
Cc: llvm cfe
<br>
Subject: Re: r228382 - OpenCL: handle shift operator with
vector operands
<br>
<br>
Hi Sameer,
<br>
<br>
This commit breaks augmented assignment in combination
with shifting.
<br>
<br>
Consider the following:
<br>
<br>
typedef __attribute__((ext_vector_type(3))) char char3;
<br>
<br>
void foo() {
<br>
char3 v = {1,1,1};
<br>
char3 w = {1,2,3};
<br>
<br>
w <<= v;
<br>
}
<br>
<br>
If I compile with:
<br>
<br>
clang -x cl file.c
<br>
<br>
Then an error is produced:
<br>
<br>
file.c:10:5: error: expression is not assignable
<br>
w <<= v;
<br>
~ ^
<br>
1 error generated.
<br>
<br>
This while the OpenCL 1.2 spec says that “w <<= v”
is short for “w = w << v”,
<br>
and which does not produce an error.
<br>
<br>
Thanks,
<br>
<br>
Jeroen
<br>
<br>
<blockquote class=" cite" id="Cite_9282131" type="cite">On
06 Feb 2015, at 05:44, Sameer Sahasrabuddhe
<sameer.sahasrabuddhe atamd.com
<a class="moz-txt-link-rfc2396E" href="http://amd.com"><http://amd.com></a>> wrote:
<br>
<br>
Author: sameerds
<br>
Date: Thu Feb 5 23:44:55 2015
<br>
New Revision: 228382
<br>
<br>
URL:<a class="moz-txt-link-freetext" href="http://llvm.org/viewvc/llvm-project?rev=228382&view=rev">http://llvm.org/viewvc/llvm-project?rev=228382&view=rev</a>
<br>
Log:
<br>
OpenCL: handle shift operator with vector operands
<br>
<br>
Introduce a number of checks:
<br>
1. If LHS is a scalar, then RHS cannot be a vector.
<br>
2. Operands must be of integer type.
<br>
3. If both are vectors, then the number of elements must
match.
<br>
<br>
Relax the requirement for "usual arithmetic
conversions":
<br>
When LHS is a vector, a scalar RHS can simply be
expanded into a
<br>
vector; OpenCL does not require that its rank be lower
than the LHS.
<br>
For example, the following code is not an error even if
the implicit
<br>
type of the constant literal is "int".
<br>
<br>
char2 foo(char2 v) { return v << 1; }
<br>
<br>
Consolidate existing tests under CodeGenOpenCL, and add
more tests
<br>
under SemaOpenCL.
<br>
<br>
<br>
Modified:
<br>
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
<br>
cfe/trunk/lib/Sema/SemaExpr.cpp
<br>
cfe/trunk/test/CodeGenOpenCL/shifts.cl
<br>
cfe/trunk/test/SemaOpenCL/shifts.cl
<br>
<br>
Modified:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
<br>
URL:<a class="moz-txt-link-freetext" href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=228382&r1=228381&r2=228382&view=diff">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=228382&r1=228381&r2=228382&view=diff</a>
<br>
==============================================================================
<br>
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
(original)
<br>
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
Thu Feb 5 23:44:55 2015
<br>
@@ -2031,6 +2031,8 @@ def
err_typecheck_vector_not_convertable
<br>
"can't convert between vector values of different size
(%0 and %1)">;
<br>
def err_typecheck_vector_not_convertable_non_scalar :
Error<
<br>
"can't convert between vector and non-scalar values
(%0 and %1)">;
<br>
+def err_typecheck_vector_lengths_not_equal : Error<
<br>
+ "vector operands do not have the same number of
elements (%0 and %1)">;
<br>
def err_ext_vector_component_exceeds_length : Error<
<br>
"vector component access exceeds type %0">;
<br>
def err_ext_vector_component_name_illegal : Error<
<br>
@@ -4884,6 +4886,8 @@ def err_ivar_reference_type :
Error<
<br>
"instance variables cannot be of reference type">;
<br>
def err_typecheck_illegal_increment_decrement :
Error<
<br>
"cannot %select{decrement|increment}1 value of type
%0">;
<br>
+def err_typecheck_expect_int : Error<
<br>
+ "used type %0 where integer is required">;
<br>
def err_typecheck_arithmetic_incomplete_type : Error<
<br>
"arithmetic on a pointer to an incomplete type
%0">;
<br>
def err_typecheck_pointer_arith_function_type :
Error<
<br>
@@ -5047,6 +5051,9 @@ def
warn_null_in_comparison_operation :
<br>
"comparison between NULL and non-pointer "
<br>
"%select{(%1 and NULL)|(NULL and %1)}0">,
<br>
InGroup<NullArithmetic>;
<br>
+def err_shift_rhs_only_vector : Error<
<br>
+ "requested shift is a vector of type %0 but the first
operand is not a "
<br>
+ "vector (%1)">;
<br>
<br>
def warn_logical_not_on_lhs_of_comparison : Warning<
<br>
"logical not is only applied to the left hand side of
this comparison">,
<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaExpr.cpp
<br>
URL:<a class="moz-txt-link-freetext" href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=228382&r1=228381&r2=228382&view=diff">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=228382&r1=228381&r2=228382&view=diff</a>
<br>
==============================================================================
<br>
--- cfe/trunk/lib/Sema/SemaExpr.cpp (original)
<br>
+++ cfe/trunk/lib/Sema/SemaExpr.cpp Thu Feb 5 23:44:55
2015
<br>
@@ -7772,6 +7772,67 @@ static void
DiagnoseBadShiftValues(Sema&
<br>
<< RHS.get()->getSourceRange();
<br>
}
<br>
<br>
+/// \brief Return the resulting type when an OpenCL
vector is shifted
<br>
+/// by a scalar or vector shift amount.
<br>
+static QualType checkOpenCLVectorShift(Sema &S,
<br>
+ ExprResult
&LHS, ExprResult &RHS,
<br>
+ SourceLocation
Loc) {
<br>
+ // OpenCL v1.1 s6.3.j says RHS can be a vector only
if LHS is a vector.
<br>
+ if (!LHS.get()->getType()->isVectorType()) {
<br>
+ S.Diag(Loc, diag::err_shift_rhs_only_vector)
<br>
+ << RHS.get()->getType() <<
LHS.get()->getType()
<br>
+ << LHS.get()->getSourceRange() <<
RHS.get()->getSourceRange();
<br>
+ return QualType();
<br>
+ }
<br>
+
<br>
+ LHS = S.UsualUnaryConversions(LHS.get());
<br>
+ if (LHS.isInvalid()) return QualType();
<br>
+
<br>
+ RHS = S.UsualUnaryConversions(RHS.get());
<br>
+ if (RHS.isInvalid()) return QualType();
<br>
+
<br>
+ QualType LHSType = LHS.get()->getType();
<br>
+ const VectorType *LHSVecTy =
LHSType->getAs<VectorType>();
<br>
+ QualType LHSEleType = LHSVecTy->getElementType();
<br>
+
<br>
+ // Note that RHS might not be a vector.
<br>
+ QualType RHSType = RHS.get()->getType();
<br>
+ const VectorType *RHSVecTy =
RHSType->getAs<VectorType>();
<br>
+ QualType RHSEleType = RHSVecTy ?
RHSVecTy->getElementType() : RHSType;
<br>
+
<br>
+ // OpenCL v1.1 s6.3.j says that the operands need to
be integers.
<br>
+ if (!LHSEleType->isIntegerType()) {
<br>
+ S.Diag(Loc, diag::err_typecheck_expect_int)
<br>
+ << LHS.get()->getType() <<
LHS.get()->getSourceRange();
<br>
+ return QualType();
<br>
+ }
<br>
+
<br>
+ if (!RHSEleType->isIntegerType()) {
<br>
+ S.Diag(Loc, diag::err_typecheck_expect_int)
<br>
+ << RHS.get()->getType() <<
RHS.get()->getSourceRange();
<br>
+ return QualType();
<br>
+ }
<br>
+
<br>
+ if (RHSVecTy) {
<br>
+ // OpenCL v1.1 s6.3.j says that for vector types,
the operators
<br>
+ // are applied component-wise. So if RHS is a
vector, then ensure
<br>
+ // that the number of elements is the same as
LHS...
<br>
+ if (RHSVecTy->getNumElements() !=
LHSVecTy->getNumElements()) {
<br>
+ S.Diag(Loc,
diag::err_typecheck_vector_lengths_not_equal)
<br>
+ << LHS.get()->getType() <<
RHS.get()->getType()
<br>
+ << LHS.get()->getSourceRange()
<< RHS.get()->getSourceRange();
<br>
+ return QualType();
<br>
+ }
<br>
+ } else {
<br>
+ // ...else expand RHS to match the number of
elements in LHS.
<br>
+ QualType VecTy =
<br>
+ �</blockquote></blockquote></blockquote></blockquote></blockquote></blockquote><br><div>[Alkuperäinen viesti ei sisälly mukaan kokonaisuudessaan.]</div></body></html>