[SYCL] Support for load/store cache controls #11584

rdeodhar · 2023-10-18T22:12:32Z

This change adds SYCL language support for load store cache controls using SPIR-V ops.

sycl/doc/extensions/proposed/sycl_ext_intel_cache_controls.asciidoc

bader · 2023-10-18T22:20:40Z

sycl/doc/extensions/proposed/sycl_ext_intel_cache_controls.asciidoc

    streaming,
    invalidate_after_read,
-    const_cached
+    const_cached,


What is the value of adding a coma here?

No real value. Can be removed. Some enums in the compiler tend to use dangling commas.

llvm/lib/SYCLLowerIR/CompileTimePropertiesPass.cpp

bader · 2023-10-18T22:37:03Z

llvm/lib/SYCLLowerIR/CompileTimePropertiesPass.cpp

+/// Builds a metadata node for a SPIR-V decoration for cache controls
+/// where decoration code and value are both uint32_t integers.
+/// The value encodes a cache level and a cache control type.


Is there a design document or specifications for LLVM/SPIR-V levels?
Cache controls are using annotated pointer design. Right?

The SPIR-V extension is here: https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/INTEL/SPV_INTEL_cache_controls.asciidoc

This implementation builds upon annotated_ptr by adding new properties.

Thanks. I also find KhronosGroup/SPIRV-LLVM-Translator#2140, which documents the LLVM IR representation of cache controls.

asudarsa · 2023-10-21T02:37:19Z

Is this related to #11597 (prefetch hints) in any way? Thanks

asudarsa · 2023-10-21T02:40:42Z

llvm/lib/SYCLLowerIR/CompileTimePropertiesPass.cpp

+/// where decoration code and value are both uint32_t integers.
+/// The value encodes a cache level and a cache control type.
+///
+/// @param Ctx    [in] the LLVM Context.


The comment does not seem to match the signature. Can you pls check? Thanks

asudarsa · 2023-10-21T02:43:23Z

llvm/lib/SYCLLowerIR/CompileTimePropertiesPass.cpp

+    streaming = 3
+  };
+  uint32_t CacheProp;
+  if (Name == "sycl-cache-read-uncached")


May be use StringSwitch here?

Pennycook · 2023-10-25T14:15:54Z

llvm/lib/SYCLLowerIR/CompileTimePropertiesPass.cpp

+    cached,
+    streaming,
+    invalidate,
+    const_cached,


How about:

Suggested change

const_cached,

constant

cache_mode::const_cached has some redundancy compared to cache_mode::constant.

Pennycook · 2023-10-25T14:19:50Z

llvm/lib/SYCLLowerIR/CompileTimePropertiesPass.cpp

+    through,
+    back


I'd prefer:

Suggested change

through,

back

write_through,

write_back

I think including "write" in the name here would help readability. These are established terms for how caches work, whereas "through" and "back" by themselves are not. Including "write" in the name here also makes it clear that passing these two modes through to read_hint will give an error.

These are not user-visible, but I'll make the change you suggest.

KseniyaTikhomirova

LGTM

sycl/include/sycl/ext/intel/experimental/cache_control_properties.hpp

sycl/doc/extensions/proposed/sycl_ext_intel_cache_controls.asciidoc

bader · 2023-11-07T00:57:59Z

llvm/lib/SYCLLowerIR/CompileTimePropertiesPass.cpp

+    write_back = 2,
+    write_streaming = 3
+  };
+  // SYCL encodings of read/write control


Please, add a warning that definition of cache_mode enum must match the definition from sycl/include/sycl/ext/intel/experimental/cache_control_properties.hpp.
Ideally, we should have a single definition across all source files.

bader · 2023-11-07T00:58:27Z

sycl/include/sycl/ext/intel/experimental/cache_control_properties.hpp

+namespace intel {
+namespace experimental {
+
+enum class cache_mode {


Please, add a warning that definition of cache_mode enum must match the definition from llvm/lib/SYCLLowerIR/CompileTimePropertiesPass.cpp.

…iidoc Co-authored-by: Alexey Bader <[email protected]>

…hectrl

…iidoc Co-authored-by: Alexey Bader <[email protected]>

…hectrl

againull · 2023-11-09T19:04:40Z

@intel/dpcpp-tools-reviewers Could you please give your +1 considering that @rdeodhar addressed comments from @asudarsa and seems that @asudarsa is on vacation.

MrSidims

CompileTimePropertiesPass changes as well as the result IR in the test seems legit

rdeodhar added 6 commits August 30, 2023 14:23

[SYCL] Proposed cache control properties for annotated_ptr.

f19cdb3

Updated doc based on review comments.

e80276e

Cleanup of doc.

81d91c7

Formatting changes.

8725dfc

[SYCL] Support for cache control properties on annotated_ptr.

b994128

Merge branch 'sycl' of https://github.com/intel/llvm into cachectrl

21a78ec

rdeodhar requested review from a team as code owners October 18, 2023 22:12

rdeodhar requested a review from KseniyaTikhomirova October 18, 2023 22:12

Formatting change.

e097acb

rdeodhar had a problem deploying to WindowsCILock October 18, 2023 22:25 — with GitHub Actions Failure

bader reviewed Oct 18, 2023

View reviewed changes

rdeodhar had a problem deploying to WindowsCILock October 18, 2023 23:07 — with GitHub Actions Failure

asudarsa reviewed Oct 21, 2023

View reviewed changes

rdeodhar added 3 commits October 23, 2023 11:15

Merge branch 'sycl' of https://github.com/intel/llvm into cachectrl

b21d9c2

Change in syntax.

f847ea3

Merge branch 'sycl' of https://github.com/intel/llvm into cachectrl

c0eab2c

rdeodhar had a problem deploying to WindowsCILock October 24, 2023 23:34 — with GitHub Actions Failure

rdeodhar requested review from Pennycook, asudarsa, gmlueck and bader October 24, 2023 23:34

rdeodhar had a problem deploying to WindowsCILock October 25, 2023 00:26 — with GitHub Actions Failure

Added missing #include.

0a54f88

rdeodhar had a problem deploying to WindowsCILock October 25, 2023 02:07 — with GitHub Actions Failure

rdeodhar temporarily deployed to WindowsCILock October 25, 2023 03:11 — with GitHub Actions Inactive

Pennycook requested changes Oct 25, 2023

View reviewed changes

rdeodhar had a problem deploying to WindowsCILock November 4, 2023 21:48 — with GitHub Actions Failure

rdeodhar had a problem deploying to WindowsCILock November 4, 2023 22:33 — with GitHub Actions Failure

Test update.

ea7e677

rdeodhar had a problem deploying to WindowsCILock November 5, 2023 00:11 — with GitHub Actions Failure

rdeodhar temporarily deployed to WindowsCILock November 5, 2023 00:51 — with GitHub Actions Inactive

Fixed merge error.

5ef494d

rdeodhar temporarily deployed to WindowsCILock November 5, 2023 20:01 — with GitHub Actions Inactive

rdeodhar temporarily deployed to WindowsCILock November 5, 2023 20:41 — with GitHub Actions Inactive

KseniyaTikhomirova approved these changes Nov 6, 2023

View reviewed changes

sycl/include/sycl/ext/intel/experimental/cache_control_properties.hpp Outdated Show resolved Hide resolved

Removed some unused declarations.

fc87598

rdeodhar temporarily deployed to WindowsCILock November 6, 2023 18:49 — with GitHub Actions Inactive

rdeodhar temporarily deployed to WindowsCILock November 6, 2023 21:06 — with GitHub Actions Inactive

bader approved these changes Nov 7, 2023

View reviewed changes

rdeodhar and others added 4 commits November 6, 2023 20:50

Update sycl/doc/extensions/proposed/sycl_ext_intel_cache_controls.asc…

abf4bcf

…iidoc Co-authored-by: Alexey Bader <[email protected]>

Update sycl/doc/extensions/proposed/sycl_ext_intel_cache_controls.asc…

b9ae23c

…iidoc Co-authored-by: Alexey Bader <[email protected]>

Update sycl/doc/extensions/proposed/sycl_ext_intel_cache_controls.asc…

904190a

…iidoc Co-authored-by: Alexey Bader <[email protected]>

Merge branch 'sycl' of https://github.com/intel/llvm into cachectrl

34f9bc6

rdeodhar temporarily deployed to WindowsCILock November 7, 2023 03:54 — with GitHub Actions Inactive

rdeodhar and others added 4 commits November 6, 2023 19:55

Merge branch 'cachectrl' of https://github.com/rdeodhar/llvm into cac…

6981032

…hectrl

Update sycl/doc/extensions/proposed/sycl_ext_intel_cache_controls.asc…

11e7887

…iidoc Co-authored-by: Alexey Bader <[email protected]>

Merge branch 'cachectrl' of https://github.com/rdeodhar/llvm into cac…

d44e645

…hectrl

Added comments noting need for consistency in cache_mode definition.

694676b

rdeodhar temporarily deployed to WindowsCILock November 7, 2023 04:25 — with GitHub Actions Inactive

rdeodhar temporarily deployed to WindowsCILock November 7, 2023 05:14 — with GitHub Actions Inactive

againull requested a review from a team November 9, 2023 19:03

rdeodhar requested review from MrSidims and removed request for a team November 9, 2023 19:04

MrSidims approved these changes Nov 10, 2023

View reviewed changes

againull merged commit 25c8b70 into intel:sycl Nov 10, 2023
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Support for load/store cache controls #11584

[SYCL] Support for load/store cache controls #11584

rdeodhar commented Oct 18, 2023

bader Oct 18, 2023

rdeodhar Oct 18, 2023

bader Oct 18, 2023

rdeodhar Oct 18, 2023

bader Oct 18, 2023

asudarsa commented Oct 21, 2023

asudarsa Oct 21, 2023

rdeodhar Oct 24, 2023

asudarsa Oct 21, 2023

Pennycook Oct 25, 2023

rdeodhar Oct 25, 2023

Pennycook Oct 25, 2023

rdeodhar Oct 25, 2023

KseniyaTikhomirova left a comment

bader Nov 7, 2023

rdeodhar Nov 7, 2023

bader Nov 7, 2023

rdeodhar Nov 7, 2023

againull commented Nov 9, 2023 •

edited

Loading

MrSidims left a comment

[SYCL] Support for load/store cache controls #11584

[SYCL] Support for load/store cache controls #11584

Conversation

rdeodhar commented Oct 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asudarsa commented Oct 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KseniyaTikhomirova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

againull commented Nov 9, 2023 • edited Loading

MrSidims left a comment

Choose a reason for hiding this comment

againull commented Nov 9, 2023 •

edited

Loading