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

[• ichesnokov]

Summary

Please review and commit changes.

[asl]

test/CodeGen/opencl-RSH-signed.c

2 ↗

(On Diff #45673)

This won't work. You need some sort of -verify in order to "expected-no-disagnostics" to work.

[asl]

lib/CodeGen/CGExprScalar.cpp

2716

Is this an OpenCL specific thing? If yes - it should at least be guarded. I believe this is the case when C99 and OpenCL disagrees.

Also, please familiarize yourself with http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface (e.g. you need to create more context in the diff, etc.).

test/CodeGenOpenCL/opencl_rhs_signed.cl

1 ↗

(On Diff #45958)

Why you're having non-OpenCL-specific test in OpenCL dir?

6 ↗

(On Diff #45958)

And? You need to check for the generated IR at least.

[• ichesnokov]

Added IR checking.

[• ichesnokov]

lib/CodeGen/CGExprScalar.cpp

2716

Yes, it's not OpenCL specific. Changing r-operator type signed to unsigned while performing << is the bug (despite that negative RHS is undefined for C++).

[asl]

lib/CodeGen/CGExprScalar.cpp

2716

Why it's a bug for non-OpenCL? Please elaborate (citing the standard, if possible).

test/CodeGen/rhs_signed_check.c

6

Do you really need to check for all these stuff? Can't you simply check only for the instructions which are relevant for your patch?

[• ichesnokov]

test/CodeGen/rhs_signed_check.c

6

I analyzed IR and didn't find a no one line to remove.

[asl]

test/CodeGen/rhs_signed_check.c

6

Sorry, what? The main difference in the generated IR before and after patch is zext => sext before shl. Everything else is heavily irrelevant.

[• ichesnokov]

IR code block reduced in test suite.

[• ichesnokov]

test/CodeGen/rhs_signed_check.c

6

IR Code portion reduced in patch.

[• ichesnokov]

BugZilla page: https://llvm.org/bugs/show_bug.cgi?id=10002
Please review and commit.

[• ichesnokov]

This comment was removed by • ichesnokov.

[• ichesnokov]

Please review and commit.

[• ichesnokov]

Please review and commit.

[• ichesnokov]

BugZilla page: https://llvm.org/bugs/show_bug.cgi?id=10002

[• ichesnokov]

Ping.

[• ichesnokov]

lib/CodeGen/CGExprScalar.cpp

2716

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

[rjmccall]

lib/CodeGen/CGExprScalar.cpp

2716

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.

lib/CodeGen/CGExprScalar.cpp

   // LLVM requires the LHS and RHS to be the same type: promote or truncate the
   // RHS to the same size as the LHS.
   Value *RHS = Ops.RHS;
-  if (Ops.LHS->getType() != RHS->getType())
-    RHS = Builder.CreateIntCast(RHS, Ops.LHS->getType(), false, "sh_prom");
+  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");
+  }
 
   bool SanitizeBase = CGF.SanOpts.has(SanitizerKind::ShiftBase) &&
                       Ops.Ty->hasSignedIntegerRepresentation();

test/CodeGen/rhs_signed_check.c

+// RUN: %clang_cc1 %s -emit-llvm -O0 -o - | FileCheck %s
+
+long long shl_long_short(long long arg1, short arg2) {
+   return (arg1 << arg2); 
+// CHECK:   %conv = sext i16 %1 to i32
+// CHECK:   %sh_prom = sext i32 %conv to i64
+// CHECK:   %shl = shl i64 %0, %sh_prom
+}