[SYCL][Joint Matrix Spec] Add new API for out of bounds fill/load/store

[dkhaldi]

Code example to show usage can be found here:
https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/Matrix/joint_matrix_out_bounds.cpp

[dkhaldi]

@gmlueck, gentle ping to review.

[gmlueck]

I don't quite understand how this API can work. We might need to schedule a meeting where we can draw some pictures.

Wouldn't it make more sense to require the user to set Width and Height according to the remaining amount of memory in the "big matrix"? That way, the implementation simply checks to see if the template parameter Rows is greater than Height and if the template parameter Cols is greater than Width. If they are both less, the operations behave like normal loads and stores. If they are less, then the load sets the out-of-range elements to zero and the store ignores the out-of-range elements.

From the description, though, it doesn't seem like the APIs work this way.

[dkhaldi]

The check is done compared to the big matrix size comparing the address to be loaded (ptr) with the big matrix size (Height). If ptr > Height, the element should not be loaded.
if the remainder is passed, how is the implementation going to compare with it?

[gmlueck]

With your recent change that adds coordX and coordY parameters, I see how this could work for the load and store APIs. However, I still don't see how the fill API can work.

This API structure seems confusing to me, though, and probably less efficient. I suspect users will get confused that the src pointer to joint_matrix_load is completely different from the src_base pointer to joint_matrix_load_checked. It seems like it would be less confusing and more efficient to have the user pass values for Width and Height that are the remaining amount of memory in the big matrix. Maybe this would be easier to explain in a meeting where we can draw pictures?

[dkhaldi]

Yes, let's discuss fill and remainder approach in a meeting.

[MrSidims]

May I unresolve this comment as there was no follow up and I probably has missed the meeting? :) I also don't understand, why we need coordinates for fill

[dkhaldi]

@fda0 does this change address the missing arguments to generate efficiently 2d load/store in IGC?

[dkhaldi]

@fda0 Yes I changed it to add the two offset coordinates separately and excluded them from the base pointer calculation.

[dkhaldi]

Notes after online discussion with @gmlueck:

• Annotated ptr load/store overloads needs to be in oneapi namespace for use on targets that support cache control hints
• Prefetch needs to be in oneapi namespace
• Out of bounds support:
  - Do we need emulation when hardware cannot do the bounds check?
  - Do we need these for AMX?
  - Fill does not need base pointer but will need coordinates and stride to be added
  - Should we add base ptr, coord x and y to the current joint_matrix_load/store to match the checked variants?

[dkhaldi]

@gmlueck, I changed the API to stick with offset pointer, no separate coordinates are needed. We did that because we found that alignment restriction does not really make a difference whether we use offset or base pointer (4 bytes alignments must be honored regardless).

However, we found an another issue if we stick with offset pointer which is the inability to support negative coordinates. There are use cases where users need to read backwards in memory and the extra elements (backward before the start of the memory pointer) need to be zeroed. If we stick with offset pointer (remainder Width and Height cannot express the negative coordinates case), we will have to restrict the use of these API to only positive coordinates which can be limiting to the real use cases.

[uphoffc]

Hi, I was asked to describe my use case for block2d loads that popped up in a related discussion about intel_subgroup_block_* functions (in OpenCL).

I am working on a kernel which requires the following sum:

$$out(..., j, ...) = \sum_i in(..., j + i - 1, ...)$$

with the boundary condition that in(k_1, ..., k_N) = 0 if any k_n is out-of-bounds.

Using the base address + coordinate format I can simply write

int2 coord = (int2)(0, i + j - 1);
ushort16 tmp = intel_subgroup_block_read_u16_m8k16v2(base_address, SurfaceWidth * sizeof(short), SurfaceHeight, SurfacePitch * sizeof(short), coord);

and then a 0 will be return if coord.x < 0 or coord.x >= SurfaceWidth.

Suggestion about the interface: Maybe it would be nice to wrap the surface parameters in its own type. Then it would be imho clearer to what Height, Width, Stride, Layout relate to. E.g. something like this:

auto big_matrix = big_matrix_ref{base_ptr, Stride, Layout, Height, Width};
void joint_matrix_load_checked(sub_group, the_joint_matrix, big_matrix, int2(coordX, coordY));

Would also require less characters to write if one has several loads on the same matrix.

We did that because we found that alignment restriction does not really make a difference whether we use offset or base pointer (4 bytes alignments must be honored regardless).

There's also a 64 B alignment requirement on the base_ptr on current gen.

[uphoffc]

Here's another thing to consider.

Assume that I tile my row-major MxN matrix into 8x16 tiles. Using the current proposal, writing a loop accessing every tile looks like the following:
1)

for (int m = 0; m < M; m += 8) {
  for (int n = 0; n < N; n += 16) {
    joint_matrix_load_checked(sub_group, result, base_ptr + n + m * Stride, Stride, M - m, N - n);
  }
}

In contrast, with base_ptr + coordinate we have
2)

for (int m = 0; m < M; m += 8) {
  for (int n = 0; n < N; n += 16) {
    joint_matrix_load_checked(sub_group, result, base_ptr, Stride, M, N, int2{m, n});
  }
}

In variant 1) we have additional arithmetic going on; we need 4 add / sub and 1 mul instruction, that would otherwise be done in hardware in a single load instruction. Moreover, we have to do the offset calculation ourselves every time and must not forget to adjust the bounds - imho much more error prone and complicated than 2).

[dkhaldi]

@gmlueck, as we discussed on Monday, I put back the coords arguments and base pointer as they are needed for negative coordinates case.
Can you please review/approve?

[MrSidims]

I believe we don't need coordinate parameters for joint_matrix_fill_checked, as we don't need to access memory.

[dkhaldi]

@MrSidims,
In the load/store, the check does the following (let's just consider one horizontal dimension to simplify):
if (base_ptr + coordY + Cols > base_ptr+Width) --> out of bounds

Since we intend to do the check just like load and store (just considering base_ptr=0), we need to add these arguments: Height, Width, Stride, CoordX, and CoordY. The check will do the following:
if (coordY + Cols > Width) --> out of bounds

[MrSidims]

May I unresolve this comment as there was no follow up and I probably has missed the meeting? :) I also don't understand, why we need coordinates for fill

[MrSidims]

LGTM with the assumption, that Stride is removed form matrilx_fill API

[dkhaldi]

LGTM with the assumption, that Stride is removed form matrilx_fill API

Yes, I just removed stride. So the expectation is the following WRT joint_matrix_fill_checked:

• Check for the y dimension (columns): coordy+smallmatrixcols >? width

• Check for the x dimension (rows): coordx + smallmatrixrows>? Height

[gmlueck]

You also added support for annotated_ptr in #11473, but that PR doesn't allow the alignment property. Is that an oversight?

[dkhaldi]

I will remove alignment from the checked APIs because only cache control is what is being considered for implementation so far.
Alignments will be handled by the query like you suggested in your last comment. So users must make sure their pointers are 4 bytes aligned. Otherwise, results are undefined.

[gmlueck]

I found this information in an email conversation we had back in January:

There are two issues with this feature because we only plan on adding it on hardware that offers this capability, no emulation:

• This will only be supported on PVC and FS1, not the CPU(AMX). So we should add a query value about OOB checking.
• They need to be aligned by 4 bytes because we want to avoid runtime checking. I will add a note about this requirement. We may need to add this as an extension to the OOB query value. Something like:
  • bool info::device::matrix::out_of_bounds --> support for OOB or not
  • size_t info::device::matrix::out_of_bounds_alignment --> alignment requirement

I'm not sure if these limitations still exist. If so, I think these queries need to be added to the spec.

Rather than adding info::device::matrix::out_of_bounds, we should add an aspect for this. Maybe name it like ext_intel_matrix_checked, as this aligns with the names of the APIs (e.g. joint_matrix_load_checked).

None of the information descriptor queries in sycl_ext_oneapi_matrix have a namespace like info::device::matrix::FOO, so I don't think you should add that namespace just for this one query. Maybe name it like ext::intel::experimental::info::device::matrix_checked_alignment.

[dkhaldi]

@gmlueck, I addressed your comments, specifically:

• Add new aspect for the checked APIs ext_intel_matrix_checked
• Restructuring and more clarifications: add subsections for aspect, introduction, API, restrictions and query
• Add Device Information Descriptors for the checked APIs restrictions

Can you please take a look again?

[dkhaldi]

@intel/llvm-gatekeepers, please help merge.