<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>