This is an archive of the discontinued LLVM Phabricator instance.

Bug 15685 - OpenCL 'char' is not signed
Needs ReviewPublic

Authored by • ichesnokov on Jan 26 2016, 3:28 AM.

Download Raw Diff

Details

Reviewers

kraiskil
asl
arsenm

Summary

Only test case is provided, no patch.
It works fine in current version of LLVM.

Diff Detail

Repository: rL LLVM

Event Timeline

• ichesnokov updated this revision to Diff 45966.Jan 26 2016, 3:28 AM

• ichesnokov retitled this revision from to A test case to ensure that char type is signed.

• ichesnokov updated this object.

• ichesnokov added reviewers: asl, kraiskil.

• ichesnokov set the repository for this revision to rL LLVM.

asl added inline comments.Jan 26 2016, 3:36 AM

test/CodeGenOpenCL/15685 - OpenCL 'char' is not signed.patch
1 ↗	(On Diff #45966)	This certainly should be a Sema check, not codegen.

• ichesnokov updated this revision to Diff 45977.Jan 26 2016, 5:24 AM

asl added inline comments.Jan 26 2016, 5:29 AM

test/SemaOpenCL/ocl-char-is-not-signed.cl
1	Why it's not OpenCL-specific?

• ichesnokov added inline comments.Jan 27 2016, 2:40 AM

test/SemaOpenCL/ocl-char-is-not-signed.cl
1	OpenCL C standard table 6.1.1: char A signed two’s complement 8-bit integer. The test is put in OpenCL folder, leaving possibility to have char unsigned in other platforms.

asl added inline comments.Jan 27 2016, 3:30 AM

test/SemaOpenCL/ocl-char-is-not-signed.cl
1	How the folder would enforce OpenCL mode? Why the test won't fail on unsigned targets?

• ichesnokov added inline comments.Jan 27 2016, 3:38 AM

test/SemaOpenCL/ocl-char-is-not-signed.cl
1	I think SemaOpenCL is the folder where from CL and only CL tests run. On unsigned targets it wouldn't run, it runs only on OpenCL. Please correct me if I am wrong.

asl added inline comments.Jan 27 2016, 3:54 AM

test/SemaOpenCL/ocl-char-is-not-signed.cl
1	You're certainly wrong. For now the test would magically work because of .cl extension which will switch the driver in OpenCL mode. No folder name is involved. In order to make this test proper you need to check that no targer-specific information would "leak" in OpenCL mode, even if you force unsigned char by default. Also, please familiarize yourself with testing docs (http://llvm.org/docs/TestingGuide.html#regression-test-structure)

• ichesnokov added inline comments.Jan 27 2016, 4:05 AM

test/SemaOpenCL/ocl-char-is-not-signed.cl
1	Ok, will know. Then there's no problem. Patch is test/SemaOpenCL/ocl-char-is-not-signed.cl and has .cl extension

• ichesnokov retitled this revision from A test case to ensure that char type is signed to Bug 15685 - OpenCL 'char' is not signed.Jan 30 2016, 9:42 AM

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

Please review and commit.

• ichesnokov added a reviewer: arsenm.Feb 3 2016, 5:51 AM

Please review and approve.

Patch improvements

Please review and accept.

Anastasia added a subscriber: Anastasia.Feb 4 2016, 10:57 AM

I am missing to understand the intention of this change. Are you trying to improve the testing for OpenCL?

No, just make test case more 'standard'.

Instead of checking by division by zero, I've used _StaticAssert. Functionally bo change, but more understandable.

• ichesnokov added a reviewer: Anastasia.Feb 8 2016, 1:00 AM

• ichesnokov removed a subscriber: Anastasia.

Anastasia resigned from this revision.Jul 12 2016, 10:14 AM

Anastasia removed a reviewer: Anastasia.

arsenm resigned from this revision.Aug 3 2017, 4:56 PM

Revision Contents

Path

Size

test/

SemaOpenCL/

	ocl-char-is-not-signed.cl
	ocl-char-is-not-signed.cl (nonexistent)

3 lines

Diff 45977

test/SemaOpenCL/ocl-char-is-not-signed.cl

				// RUN: %clang_cc1 %s
				aslUnsubmitted Not Done Reply Inline Actions Why it's not OpenCL-specific? asl: Why it's not OpenCL-specific?
				ichesnokovAuthorUnsubmitted Not Done Reply Inline Actions OpenCL C standard table 6.1.1: char A signed two’s complement 8-bit integer. The test is put in OpenCL folder, leaving possibility to have char unsigned in other platforms. ichesnokov: OpenCL C standard table 6.1.1: char A signed two’s complement 8-bit integer. The test is put…
				aslUnsubmitted Not Done Reply Inline Actions How the folder would enforce OpenCL mode? Why the test won't fail on unsigned targets? asl: How the folder would enforce OpenCL mode? Why the test won't fail on unsigned targets?
				ichesnokovAuthorUnsubmitted Not Done Reply Inline Actions I think SemaOpenCL is the folder where from CL and only CL tests run. On unsigned targets it wouldn't run, it runs only on OpenCL. Please correct me if I am wrong. ichesnokov: I think SemaOpenCL is the folder where from CL and only CL tests run. On unsigned targets it…
				aslUnsubmitted Not Done Reply Inline Actions You're certainly wrong. For now the test would magically work because of .cl extension which will switch the driver in OpenCL mode. No folder name is involved. In order to make this test proper you need to check that no targer-specific information would "leak" in OpenCL mode, even if you force unsigned char by default. Also, please familiarize yourself with testing docs (http://llvm.org/docs/TestingGuide.html#regression-test-structure) asl: You're certainly wrong. For now the test would magically work because of .cl extension which…
				ichesnokovAuthorUnsubmitted Not Done Reply Inline Actions Ok, will know. Then there's no problem. Patch is test/SemaOpenCL/ocl-char-is-not-signed.cl and has .cl extension ichesnokov: Ok, will know. Then there's no problem. Patch is test/SemaOpenCL/ocl-char-is-not-signed.cl and…

				constant int CHECK = (char)(0xFFu) < 0 ? 0 : 1/0;