intel / intel/llvm

Cleanup pipe extension specifications

Open
#9,465 5 comments 0 reactions 1 assignee Claimed by @justin-rosner View on GitHub
confirmed enhancement
Dominant language
LLVM
Stars
1.5k
Forks
854
Avg merge
3d 17h
Merged PRs (30d)
137

Description

The current pipes extension specs (https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/supported/sycl_ext_intel_dataflow_pipes.asciidoc and https://github.com/intel/llvm/pull/9027/files?short_path=e64b9f7#diff-e64b9f7634f7f2218305a6811bcf87686e3cac717238065a83f8d74f227b3c7a) have several issues related to the experimental properties that should be cleaned up. These are the issues I see:

1. The latency controls properties are defined in the core pipe spec while the host pipe interface controls are described in a separate extension. There's no particular reason to treat these sets of properties differently (they both apply only to FPGAs for example) so they should either both be in the core spec or both be in an separate extension for consistency.
2. In the core spec, in the section about the newly added properties we say (emphasis mine):

>In the experimental API version, the device side read/write methods take in a **property list** as function argument,

This should say "properties" or "properties object". "Property list" is a class defined in the core SYCL spec that can't convey properties at compile time.

3. This sentence seems to say the opposite of what it means:
> The compiler is allowed to optimize the pipe if both sides are visible.

I believe the intent is to say that the compiler is allowed to optimize the pipe if points endpoints are visible _to it_. But by omitting those crucial words it sounds like we're saying that the compiler is allowed to ignore the annotation if both endpoints are visible side-effects of the program, i.e. if they _aren't_ visible to the compiler. I think it would be cleaner to drop this sentence entirely.

4. In the definition of bits_per_symbol we state:
> Valid Values: A positive integer value that evenly divides by the data type size.

The bits_per_symbol value needs to evenly divide the data type size, not the other way around.

5. ready_latency and bits_per_symbol are encoded as signed integers but are restricted to unsigned values. It would probably be cleaner to use unsigned integers.

7. Why must min_capacity be 0 for a pipe with uses_valid = false?

9. It doesn't make sense to lump the presence/absence of a ready signal into the protocol property but not do the same for the valid signal given that they are two sides of the same coin. Either both should be part of the protocol property of neither should be.

11. ready_latency is a property of avalon-st but not avalon-mm. What happens if I set it when the protocol is avalon_mm or avalon_mm_uses_ready. This and the previous point make me think we need to re-think how these properties interact with protocol.

1. The avalon_mm property doesn't make sense because when using a shared CSR, the protocol can't be decided on a pipe-by-pipe basis. There's only one shared CSR in such cases and as such its protocol is a device-image-level decision. As such, that property would be better renamed something to the effect of "shared CSR". We should make sure to align on this name across all things that can be accessed through the CSR (device globals, kernel arguments, kernel controls).

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.