This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Add separate read_only and write_only pipe IR types
ClosedPublic

Authored by stuart on Apr 24 2018, 8:25 AM.

Details

Summary

SPIR-V encodes the read_only and write_only access qualifiers of pipes, so separate LLVM IR types are required to target SPIR-V. Other backends may also find this useful.

These new types are opencl.pipe_ro_t and opencl.pipe_wo_t, which replace opencl.pipe_t.

This replaces __get_pipe_num_packets(...) and __get_pipe_max_packets(...) which took a read_only pipe with separate versions for read_only and write_only pipes, namely:

  • __get_pipe_num_packets_ro(...)
  • __get_pipe_num_packets_wo(...)
  • __get_pipe_max_packets_ro(...)
  • __get_pipe_max_packets_wo(...)

These separate versions exist to avoid needing a bitcast to one of the two qualified pipe types.

Diff Detail

Event Timeline

stuart created this revision.Apr 24 2018, 8:25 AM
svenvh added a subscriber: svenvh.Apr 24 2018, 8:32 AM

It is not clear why we need two versions of get_pipe_num_packets and get_pipe_max_packets builtins. There is only one instruction per builtin in the SPIR-V spec. I think splitting the IR type is enough for translation to SPIR-V purposes.

AlexeySotkin added inline comments.Apr 24 2018, 3:33 PM
lib/CodeGen/CGOpenCLRuntime.h
65

I'm not sure that it is a good idea to make this function public, as its parameter supposed to be a reference to protected member.

stuart updated this revision to Diff 143938.Apr 25 2018, 8:16 AM
stuart edited the summary of this revision. (Show Details)

Changed new getPipeType() method to have protected visibility.

Updated summary to explain the need for the extra builtin implementation functions.

There should not be need for bitcast. Could give an example ? Thanks.

It is not clear why we need two versions of get_pipe_num_packets and get_pipe_max_packets builtins. There is only one instruction per builtin in the SPIR-V spec. I think splitting the IR type is enough for translation to SPIR-V purposes.

This is so that when we emit the builtin expression, we can call a function that matches the access qualifier of the argument to the builtin, without the need for a bitcast of either the builtin's argument or the __get_pipe_max/num_packets() function itself.

lib/CodeGen/CGOpenCLRuntime.h
65

That's a good point. I have changed the function to be protected, to match the visibility of the data member.

stuart marked an inline comment as done.Apr 25 2018, 8:26 AM

There should not be need for bitcast. Could give an example ? Thanks.

If I have a write_only pipe as the argument to get_pipe_max_packets(), and this uses a single __get_pipe_num_packets() function taking a read_only pipe, we will automatically get a bitcast:

%20 = call i32 bitcast (i32 (%opencl.pipe_ro_t*, i32, i32)* @__get_pipe_max_packets to i32 (%opencl.pipe_wo_t*, i32, i32)*)(%opencl.pipe_wo_t* %19, i32 4, i32 4)

There should not be need for bitcast. Could give an example ? Thanks.

If I have a write_only pipe as the argument to get_pipe_max_packets(), and this uses a single __get_pipe_num_packets() function taking a read_only pipe, we will automatically get a bitcast:

%20 = call i32 bitcast (i32 (%opencl.pipe_ro_t*, i32, i32)* @__get_pipe_max_packets to i32 (%opencl.pipe_wo_t*, i32, i32)*)(%opencl.pipe_wo_t* %19, i32 4, i32 4)

Sorry, but I don't quite understand what does get_pipe_max_packets(), uses __get_pipe_num_packets() mean. Could you clarify? Possibly OpenCL C source example could help.
Thanks

stuart added a comment.EditedApr 25 2018, 9:51 AM

There should not be need for bitcast. Could give an example ? Thanks.

If I have a write_only pipe as the argument to get_pipe_max_packets(), and this uses a single __get_pipe_num_packets() function taking a read_only pipe, we will automatically get a bitcast:

%20 = call i32 bitcast (i32 (%opencl.pipe_ro_t*, i32, i32)* @__get_pipe_max_packets to i32 (%opencl.pipe_wo_t*, i32, i32)*)(%opencl.pipe_wo_t* %19, i32 4, i32 4)

Sorry, but I don't quite understand what does get_pipe_max_packets(), uses __get_pipe_num_packets() mean. Could you clarify? Possibly OpenCL C source example could help.

I mean that without these two separate versions, the call to __get_pipe_num_packets() that is emitted can include a bitcast.

For example:

void foo(read_only pipe int r, write_only pipe int w) {
  get_pipe_num_packets(w);
  get_pipe_num_packets(r);
}

get_pipe_num_packets(w) is seen first, causing i32 @__get_pipe_num_packets(%opencl.pipe_wo_t*, i32, i32) to be implicitly declared.

When the call to __get_pipe_num_packets() is emitted, this will be with an autogenerated bitcast from the type of the implicit declaration, i.e. i32 (%opencl.pipe_wo_t*, i32, i32)* to the type in the emitted expression, i.e. i32 (%opencl.pipe_ro_t*, i32, i32)*.

Here is the relevant section of IR:

%0 = load %opencl.pipe_wo_t*, %opencl.pipe_wo_t** %w.addr, align 8
%1 = call i32 @__get_pipe_num_packets(%opencl.pipe_wo_t* %0, i32 4, i32 4)
%2 = load %opencl.pipe_ro_t*, %opencl.pipe_ro_t** %r.addr, align 8
%3 = call i32 bitcast (i32 (%opencl.pipe_wo_t*, i32, i32)* @__get_pipe_num_packets to i32 (%opencl.pipe_ro_t*, i32, i32)*)(%opencl.pipe_ro_t* %2, i32 4, i32 4)

If we swap the two uses of get_pipe_num_packets() in the example above, then the type of the implicit declaration will be i32 (%opencl.pipe_ro_t*, i32, i32)* and bitcasts will instead be automatically generated when using get_pipe_num_packets() with a write_only pipe. It seems especially unfortunate that the type of the implicit declaration varies depending on the access qualifier of the first use.

There should not be need for bitcast. Could give an example ? Thanks.

If I have a write_only pipe as the argument to get_pipe_max_packets(), and this uses a single __get_pipe_num_packets() function taking a read_only pipe, we will automatically get a bitcast:

%20 = call i32 bitcast (i32 (%opencl.pipe_ro_t*, i32, i32)* @__get_pipe_max_packets to i32 (%opencl.pipe_wo_t*, i32, i32)*)(%opencl.pipe_wo_t* %19, i32 4, i32 4)

Sorry, but I don't quite understand what does get_pipe_max_packets(), uses __get_pipe_num_packets() mean. Could you clarify? Possibly OpenCL C source example could help.

I mean that without these two separate versions, the call to __get_pipe_num_packets() that is emitted can include a bitcast.

For example:

void foo(read_only pipe int r, write_only pipe int w) {
  get_pipe_num_packets(w);
  get_pipe_num_packets(r);
}

get_pipe_num_packets(w) is seen first, causing i32 @__get_pipe_num_packets(%opencl.pipe_wo_t*, i32, i32) to be implicitly declared.

When the call to __get_pipe_num_packets() is emitted, this will be with an autogenerated bitcast from the type of the implicit declaration, i.e. i32 (%opencl.pipe_wo_t*, i32, i32)* to the type in the emitted expression, i.e. i32 (%opencl.pipe_ro_t*, i32, i32)*.

Here is the relevant section of IR:

%0 = load %opencl.pipe_wo_t*, %opencl.pipe_wo_t** %w.addr, align 8
%1 = call i32 @__get_pipe_num_packets(%opencl.pipe_wo_t* %0, i32 4, i32 4)
%2 = load %opencl.pipe_ro_t*, %opencl.pipe_ro_t** %r.addr, align 8
%3 = call i32 bitcast (i32 (%opencl.pipe_wo_t*, i32, i32)* @__get_pipe_num_packets to i32 (%opencl.pipe_ro_t*, i32, i32)*)(%opencl.pipe_ro_t* %2, i32 4, i32 4)

If we swap the two uses of get_pipe_num_packets() in the example above, then the type of the implicit declaration will be i32 (%opencl.pipe_ro_t*, i32, i32)* and bitcasts will instead be automatically generated when using get_pipe_num_packets() with a write_only pipe. It seems especially unfortunate that the type of the implicit declaration varies depending on the access qualifier of the first use.

Oh I see. LGTM then. Thanks.

stuart edited the summary of this revision. (Show Details)Apr 27 2018, 3:31 AM
stuart edited the summary of this revision. (Show Details)Apr 27 2018, 3:33 AM
stuart edited the summary of this revision. (Show Details)Apr 27 2018, 3:37 AM
This revision was not accepted when it landed; it landed in state Needs Review.Apr 27 2018, 3:40 AM
This revision was automatically updated to reflect the committed changes.