Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Signed char extraction fails to take sign into account #46

Open
dneto0 opened this issue Aug 28, 2017 · 5 comments
Open

Signed char extraction fails to take sign into account #46

dneto0 opened this issue Aug 28, 2017 · 5 comments
Assignees
Labels
Projects

Comments

@dneto0
Copy link
Collaborator

dneto0 commented Aug 28, 2017

kernel void foo_fixed(global uint* A, global char4* B) {
 char4 val = *B;
 A[0] = val.x;
 A[1] = val.y;
 A[2] = val.z;
 A[3] = val.w;
}

Generates, in part, this code:

         %34 = OpShiftRightLogical %uint %33 %uint_0
         %35 = OpBitwiseAnd %uint %34 %uint_255
         %36 = OpSConvert %uint %34    ; This signed conversion is not enough
               OpStore %31 %36

The bug is that the LLVM i8 is converted to a SPIR-V uint, and so the SConvert is from a uint to a uint. This does not do the sign extension that LLVM expects will be implicitly done as part of the conversion.

The clspv compiler needs to insert more instructions to replicate the sign bit from position 7 to all the remainder.

@dneto0 dneto0 self-assigned this Aug 28, 2017
@dneto0
Copy link
Collaborator Author

dneto0 commented Aug 28, 2017

The root cause is the translation of

  %conv = sext i8 %1 to i32

The i8 is mapped to a SPIR-V 32-bit unsigned int. But the conversion of the SExt doesn't take that into account.

Original OpenCL C:

kernel void bar(global int* A, global char4* B) {
  *A = B->y;
}

last LLVM:

  %0 = load <4 x i8>, <4 x i8> addrspace(1)* %B, align 4
  %1 = extractelement <4 x i8> %0, i32 1
  %conv = sext i8 %1 to i32
  store i32 %conv, i32 addrspace(1)* %A, align 4

becomes SPIR-V:

         %28 = OpLoad %uint %27
         %29 = OpShiftRightLogical %uint %28 %uint_8
         %30 = OpBitwiseAnd %uint %29 %uint_255
         %31 = OpSConvert %uint %29   ; Not right because source type is too wide in SPIR-V
               OpStore %26 %31

@dneto0
Copy link
Collaborator Author

dneto0 commented Aug 28, 2017

There's a corresponding problem with unsigned char.

@dneto0
Copy link
Collaborator Author

dneto0 commented Aug 28, 2017

Also sitofp i8 to float

kernel void bar(global float* A, global char4* B) {
  *A = (float)(B->y);
}

Final LLVM:

  %1 = extractelement <4 x i8> %0, i32 1
  %conv = sitofp i8 %1 to float
  store float %conv, float addrspace(1)* %A, align 4

becomes SPIR-V:

         %29 = OpLoad %uint %28
         %30 = OpShiftRightLogical %uint %29 %uint_8
         %31 = OpBitwiseAnd %uint %30 %uint_255
         %32 = OpConvertSToF %float %30

@dneto0
Copy link
Collaborator Author

dneto0 commented Aug 28, 2017

I can't reproduce a problem with unsigned char.

@dneto0 dneto0 added this to In progress in General Aug 28, 2017
@dneto0 dneto0 added the bug label Aug 29, 2017
@dneto0 dneto0 moved this from In progress to Bugs in General Mar 4, 2019
@alan-baker
Copy link
Collaborator

Default behaviour (when char support is enabled) is ok, but disabling char support is still problematic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
General
  
Bugs
Development

No branches or pull requests

2 participants