Page MenuHomePhabricator

[OpenCL][CodeGen] Fix replacing memcpy with addrspacecast
ClosedPublic

Authored by sidorovd on Nov 27 2018, 5:34 AM.

Details

Summary

If a function argument is byval and RV is located in default or alloca address space
an optimization of creating addrspacecast instead of memcpy is performed. That is
not correct for OpenCL, where that can lead to a situation of address space casting
from private * to global *. See an example below:

typedef struct {
  int x;
} MyStruct;

void foo(MyStruct val) {}

kernel void KernelOneMember(__global MyStruct* x) {
  foo (*x);
}

for this code clang generated following IR:
...
%0 = load %struct.MyStruct addrspace(1)*, %struct.MyStruct addrspace(1)**
%x.addr, align 4
%1 = addrspacecast %struct.MyStruct addrspace(1)* %0 to %struct.MyStruct*
...

So the optimization was disallowed for OpenCL if RV is located in an address space
different than that of the argument (0).

Diff Detail

Repository
rC Clang

Event Timeline

sidorovd created this revision.Nov 27 2018, 5:34 AM
Anastasia added inline comments.Nov 28 2018, 3:35 AM
lib/CodeGen/CGCall.cpp
3976

This statement is incorrect. Private is only default AS in CL versions before 2.0.

I feel this can be expressed simpler. I guess the observation here is that if addr space mismatches it has to generate a copy because even if it would be a generic we can't get what the original specific addr space was? So it's safe to generate a copy.

test/CodeGenOpenCL/addr-space-struct-arg.cl
2

Do you know why we need -finclude-default-header here? If it's just for vector types perhaps we should just declare them here... headers parsing is expensive in terms of time.

sidorovd updated this revision to Diff 175722.Nov 28 2018, 10:16 AM
sidorovd marked 4 inline comments as done.

@yaxunl, since I'm partially reverting your change https://reviews.llvm.org/D34367 can you give a feedback on this?

lib/CodeGen/CGCall.cpp
3976

This statement is incorrect. Private is only default AS in CL versions before 2.0.

You are right, thanks!

Still I want to leave the comment almost as is, since it creates a link with a previous one on lines 3928-3935.

test/CodeGenOpenCL/addr-space-struct-arg.cl
2

Yeap, it's just for vectors. Added typedef for int2.

Anastasia added inline comments.Nov 29 2018, 9:00 AM
lib/CodeGen/CGCall.cpp
3976

I feel it doesn't add much value, but rather exactly says what code does. At least this is definitely obvious from this code:

Create memcpy call if RV is located in an address space different than that of
sidorovd updated this revision to Diff 176085.Nov 30 2018, 4:18 AM
sidorovd marked an inline comment as done.

Removed redundant lines in a comment

lib/CodeGen/CGCall.cpp
3976

Agree

Anastasia accepted this revision.Dec 3 2018, 4:03 AM

LGTM! Thanks!

This revision is now accepted and ready to land.Dec 3 2018, 4:03 AM
This revision was automatically updated to reflect the committed changes.