search

intel / llvm

Intel staging area for llvm.org contribution. Home for Intel LLVM-based projects.
Other
1.21k stars 724 forks source link

Cleanup pipe extension specifications #9465

Open GarveyJoe opened 1 year ago

GarveyJoe commented 1 year ago

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.

  1. 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.

  1. 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.

  1. 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.

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

  3. 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.

  4. 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.

  5. 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).

GarveyJoe commented 1 year ago

I don't have assignment privileges. Can this be assigned to @rrzeszut?

AlexeySachkov commented 1 year ago

Can this be assigned to @rrzeszut?

Looks like no, GitHub UI doesn't show that account even if I copy the name. Probably some extra permissions within the repo are needed for that

GarveyJoe commented 2 months ago

@justin-rosner, do you own pipes now? 1 and 2 from this list have yet to be addressed. Also, there was one other concern that Rob was aware of but wasn't on this tracker; I'll add it as number 9 and cross out the addressed ones.

justin-rosner commented 2 months ago

@justin-rosner, do you own pipes now? 1 and 2 from this list have yet to be addressed. Also, there was one other concern that Rob was aware of but wasn't on this tracker; I'll add it as number 9 and cross out the addressed ones.

I do own pipes now. I'll take a look at this.

GarveyJoe commented 2 months ago

I couldn't get the strikethrough to format nicely with the quotes, but 3-8 have been addressed in this PR: https://github.com/intel/llvm/commit/2a091189209f47235383700c1dca96461729a520