[ESIMD] Implement gather(acc) accepting compile-time properties #12414

v-klochkov · 2024-01-17T03:56:17Z

No description provided.

Signed-off-by: Vyacheslav N Klochkov <vyacheslav.n.klochkov@intel.com> Signed-off-by: Klochkov, Vyacheslav N <vyacheslav.n.klochkov@intel.com>

sarnex · 2024-01-22T19:05:02Z

sycl/include/sycl/ext/intel/esimd/memory.hpp

-gather(AccessorTy acc, simd<detail::DeviceAccessorOffsetT, N> offsets,
-       detail::DeviceAccessorOffsetT glob_offset = 0, simd_mask<N> mask = 1) {
+// Dev note: the argument \p glob_offset of this function does not have
+// the default value to not conflict with more generic variant (acc-ga-3)


Suggested change

// the default value to not conflict with more generic variant (acc-ga-3)

// a default value to not conflict with more generic variant (acc-ga-3)

sarnex · 2024-01-22T19:05:19Z

sycl/include/sycl/ext/intel/esimd/memory.hpp

-       detail::DeviceAccessorOffsetT glob_offset = 0, simd_mask<N> mask = 1) {
+// Dev note: the argument \p glob_offset of this function does not have
+// the default value to not conflict with more generic variant (acc-ga-3)
+// defined lower. This restriction though requires adding an additional


Suggested change

// defined lower. This restriction though requires adding an additional

// defined below. This restriction though requires adding an additional

sarnex · 2024-01-22T19:06:53Z

sycl/include/sycl/ext/intel/esimd/memory.hpp

 #else
-  return detail::gather_impl<T, N, AccessorTy>(acc, offsets, glob_offset, mask);
-#endif
+  if constexpr (sizeof(T) > 4 || !((N == 1 || N == 8 || N == 16 || N == 32))) {


nit you can ignore: personally i find it hard to read ! on outside of large condition, i prefer ! applied to each element, but thats just me

thank you for the comment, but here I'll keep it as is. It is read as `T is big && N is Not from set{1,8,16,32).
This comment here just preserves the old/previous checks for gather. I may need to re-visit it a bit later, perhaps N=4 is also supported for gen12+genx_intrinsics case.

sarnex · 2024-01-22T19:10:04Z

sycl/include/sycl/ext/intel/esimd/memory.hpp

+///                   PropertyListT props = {});                   // (acc-ga-2)
+/// simd<T, N> gather(AccessorT acc, simd<OffsetT, N / VS> byte_offsets,
+///                   PropertyListT props = {});                   // (acc-ga-3)
+


Nit: Whitespace difference between here and other variants

I added that white space intentionally to make it a bit more readable by splitting to groups of 3.
Actually this comment made me to think critically at this block and I noticed acc-ga-7,8,9 absent here.
I'll add them and add couple more grouping sentences to this comment/block.

sarnex · 2024-01-22T19:13:53Z

sycl/include/sycl/ext/intel/esimd/memory.hpp

+/// Supported platforms: DG2, PVC only - Temporary restriction for the variant
+/// with pass_thru operand. The only exception: DG2/PVC is not required if
+/// stateless memory mode is enforced via -fsycl-esimd-force-stateless-mem and
+/// VS == 1 and no L1/L2 cache hints passed and __ESIMD_GATHER_SCATTER_LLVM_IR


Suggested change

/// VS == 1 and no L1/L2 cache hints passed and __ESIMD_GATHER_SCATTER_LLVM_IR

/// VS == 1 and no L1/L2 cache hints passed and the __ESIMD_GATHER_SCATTER_LLVM_IR

Ok, let's have it. I added it here and in one more place (acc-ga-2)

sarnex · 2024-01-22T20:18:52Z

sycl/test-e2e/ESIMD/unified_memory_api/Inputs/gather.hpp

+               else // ByteOffset - simd_view
+                 Vals = gather<T, N>(InAcc, ByteOffsetsView, Pred);
+             }
+           } else { // UsePassThru is false, UseMask is false


This must have been very painful to write

sarnex · 2024-01-22T20:19:02Z

sycl/test-e2e/ESIMD/unified_memory_api/Inputs/gather.hpp

+           }
+         } // end if (VS == 1)
+         Vals.copy_to(Out + GlobalID * N);
+         // scatter(Out, ByteOffsets.template select<NOffsets, 1>(), Vals);


Nit: remove commented code

Ok, removed.

sarnex · 2024-01-22T20:19:23Z

sycl/test-e2e/ESIMD/unified_memory_api/Inputs/gather.hpp

+    // Check VS > 1. GPU supports only dwords and qwords in this mode.
+    if constexpr (sizeof(T) >= 4) {
+      // TODO: This test case causes flaky fail. Enable it after the issue
+      // in GPU driver is fixed.


Do we have an internal tracker for this or a version we know works?

Yes, I created a tracker before uploading this PR.
GPU generated code where the mask is used but is not initialized.

sarnex · 2024-01-22T20:20:07Z

sycl/test-e2e/ESIMD/unified_memory_api/gather_acc_dg2_pvc.cpp

+// The test verifies esimd::gather() functions accepting ACCESSOR
+// and optional compile-time esimd::properties.
+// The gather() calls in this test do not use cache-hint properties
+// or VS > 1 (number of loads per offset) to not impose using PVC features.


Commit is not accurate I think in the part about not using PVC features

Yes, fixed. Thank you.

sarnex · 2024-01-22T20:20:29Z

sycl/test-e2e/ESIMD/unified_memory_api/gather_acc_dg2_pvc.cpp

+  Passed &= testACC<uint32_t, TestFeatures>(Q);
+  Passed &= testACC<float, TestFeatures>(Q);
+  Passed &= testACC<ext::intel::experimental::esimd::tfloat32, TestFeatures>(Q);
+#ifdef __ESIMD_FORCE_STATELESS_MEM


Should we be running this test and the previous in stateless mode as well? It seems we are not right now.

Yes, good catch. I'll add the tests here soon.

did not mean to approve

Need also to add E2E tests for stateless mode Signed-off-by: Klochkov, Vyacheslav N <vyacheslav.n.klochkov@intel.com>

…her_acc

Signed-off-by: Klochkov, Vyacheslav N <vyacheslav.n.klochkov@intel.com>

v-klochkov had a problem deploying to WindowsCILock January 17, 2024 03:56 — with GitHub Actions Failure

v-klochkov had a problem deploying to WindowsCILock January 17, 2024 04:19 — with GitHub Actions Failure

v-klochkov had a problem deploying to WindowsCILock January 17, 2024 08:26 — with GitHub Actions Failure

v-klochkov had a problem deploying to WindowsCILock January 17, 2024 08:48 — with GitHub Actions Failure

v-klochkov force-pushed the esimd_unified_gather_acc branch from 33d2fbd to 515a418 Compare January 18, 2024 05:49

v-klochkov temporarily deployed to WindowsCILock January 18, 2024 05:49 — with GitHub Actions Inactive

v-klochkov had a problem deploying to WindowsCILock January 18, 2024 06:10 — with GitHub Actions Failure

v-klochkov force-pushed the esimd_unified_gather_acc branch from 515a418 to 252380c Compare January 19, 2024 02:40

v-klochkov temporarily deployed to WindowsCILock January 19, 2024 02:41 — with GitHub Actions Inactive

v-klochkov had a problem deploying to WindowsCILock January 19, 2024 03:02 — with GitHub Actions Error

v-klochkov force-pushed the esimd_unified_gather_acc branch from 252380c to 116fbdc Compare January 19, 2024 03:09

v-klochkov temporarily deployed to WindowsCILock January 19, 2024 03:09 — with GitHub Actions Inactive

v-klochkov temporarily deployed to WindowsCILock January 19, 2024 03:32 — with GitHub Actions Inactive

[ESIMD] Implement gather(acc) accepting compile-time properties

04edb74

Signed-off-by: Vyacheslav N Klochkov <vyacheslav.n.klochkov@intel.com> Signed-off-by: Klochkov, Vyacheslav N <vyacheslav.n.klochkov@intel.com>

v-klochkov force-pushed the esimd_unified_gather_acc branch from 116fbdc to 04edb74 Compare January 20, 2024 01:57

v-klochkov temporarily deployed to WindowsCILock January 20, 2024 01:58 — with GitHub Actions Inactive

v-klochkov marked this pull request as ready for review January 20, 2024 01:59

v-klochkov requested a review from a team as a code owner January 20, 2024 01:59

v-klochkov temporarily deployed to WindowsCILock January 20, 2024 02:19 — with GitHub Actions Inactive

v-klochkov requested review from sarnex and fineg74 January 22, 2024 18:33

sarnex previously approved these changes Jan 22, 2024

View reviewed changes

v-klochkov added 2 commits January 22, 2024 14:39

Address code-review comments from Nick - NFC only fixes here

01857b8

Need also to add E2E tests for stateless mode Signed-off-by: Klochkov, Vyacheslav N <vyacheslav.n.klochkov@intel.com>

Merge remote-tracking branch 'intel_llvm/sycl' into esimd_unified_gat…

3288870

…her_acc

v-klochkov temporarily deployed to WindowsCILock January 22, 2024 22:42 — with GitHub Actions Inactive

v-klochkov had a problem deploying to WindowsCILock January 22, 2024 23:05 — with GitHub Actions Error

Add 2 E2E tests verifying stateless memory mode

ca0650a

Signed-off-by: Klochkov, Vyacheslav N <vyacheslav.n.klochkov@intel.com>

v-klochkov temporarily deployed to WindowsCILock January 22, 2024 23:26 — with GitHub Actions Inactive

v-klochkov temporarily deployed to WindowsCILock January 22, 2024 23:52 — with GitHub Actions Inactive

sarnex approved these changes Jan 23, 2024

View reviewed changes

v-klochkov merged commit 655ab10 into intel:sycl Jan 23, 2024

v-klochkov deleted the esimd_unified_gather_acc branch January 23, 2024 21:33

	// the default value to not conflict with more generic variant (acc-ga-3)
	// a default value to not conflict with more generic variant (acc-ga-3)

	// defined lower. This restriction though requires adding an additional
	// defined below. This restriction though requires adding an additional

	/// VS == 1 and no L1/L2 cache hints passed and __ESIMD_GATHER_SCATTER_LLVM_IR
	/// VS == 1 and no L1/L2 cache hints passed and the __ESIMD_GATHER_SCATTER_LLVM_IR

[ESIMD] Implement gather(acc) accepting compile-time properties #12414

[ESIMD] Implement gather(acc) accepting compile-time properties #12414

Uh oh!

Conversation

v-klochkov commented Jan 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

v-klochkov Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

v-klochkov Jan 22, 2024 •

edited

Loading