[PATCH] D16460: Bug 10002 - [opencl] Wrongfully assuming RHS is always unsigned

[John McCall via cfe-commits]

rjmccall added inline comments.

================
Comment at: lib/CodeGen/CGExprScalar.cpp:2716
@@ +2715,3 @@
+  if (Ops.LHS->getType() != RHS->getType()) {
+    bool isSigned = dyn_cast<BinaryOperator>(Ops.E)->getRHS()->getType().getTypePtr()->isSignedIntegerType();
+    RHS = Builder.CreateIntCast(RHS, Ops.LHS->getType(), isSigned, "sh_prom");
----------------
ichesnokov wrote:
> The OpenCL 2.0 standard does not say anything about right operand sign of shift operators.
> 
> C standard says the behavior is undefined:
> http://www.coding-guidelines.com/cbook/cbook1_0b.pdf, 1175
> "If the value of the right operand is negative or is greater than or equal to the width of the promoted left
> operand, the behavior is undefined."
> 
> In C++11 standard said the same:
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4296.pdf, 5.8
> "The behavior is undefined if the right operand
> is negative, or greater than or equal to the length in bits of the promoted left operand"
> 
> I.e., we can close this bug as NAB and do not apply patch.
> However, I believe that passing right operand, preserving it's sign, gives more flexibility to developers. It makes opportunity to developers use signed right operator, not only unsigned (if development is working for specific platform where developer knows what happens when right operand is signed).
> 
I agree that it would be abstractly better to sign-extend the RHS if it's a signed type.  Please do this for both << and >>, and please make sure your test covers <<= and >>= as well.

Also, please use cast<> for unconditional casts instead of dyn_cast.


Repository:
  rL LLVM

http://reviews.llvm.org/D16460