Implement RegisterMapInterfaceINTEL execution mode #1774

tiwaria1 · 2022-12-08T23:03:28Z

This PR adds the new execution mode RegisterMapInterfaceINTEL, see the Khronos
SPIRV spec here: KhronosGroup/SPIRV-Registry#176
This execution mode allows specifying a 'register' based interface for FPGA kernels.

The RegisterMapInterfaceINTEL execution mode is added with a 0/1 literal based on
the kernel metadata. When the metadata is:

!ip_interface !N
!N = !{!"csr"}

The translator emits RegisterMapInterfaceINTEL 0, and when the metadata is:

!ip_interface !N
!N = !{!"csr", !"accept_downstream_stall"}

The translator emits RegisterMapInterfaceINTEL 1

NOTE

The new mode is under capability FPGAKernelAttributesv2INTEL which implicitly defines the capability FPGAKernelAttributesv2INTEL.
This PR is very similar to Implement StreamingInterfaceINTEL execution mode #1218

CLAassistant · 2022-12-08T23:03:33Z

All committers have signed the CLA.

tiwaria1 · 2022-12-08T23:05:22Z

@MrSidims Could you please help me review this as well? The tricky part here is that:

This exec mode is under capability FPGAKernelAttributesv2INTEL which implicitly defines the existing capability FPGAKernelAttributesINTEL. Not sure how exactly to reflect that in the code yet.
The spec states only one of StreamingInterfaceINTEL and RegisterMapInterfaceINTEL are valid at a time - should this be checked here in the translator or at some other level of the tool?

lib/SPIRV/SPIRVWriter.cpp

MrSidims · 2022-12-09T10:43:34Z

The spec states only one of StreamingInterfaceINTEL and RegisterMapInterfaceINTEL are valid at a time - should this be checked here in the translator or at some other level of the tool?

We usually add checks on the translation level, see Unroll vs DontUnroll masks as an example. I don't remember, if it's a case for FPGA extensions, since we were relying on clang to check correctness of attributes back then (probably we still have some checks here as well), but it seems to be a bad idea now not to add checks, since we no longer adding attributes to clang (with appropriate diagnostics). So I'd suggest to add checks here.

MrSidims · 2022-12-09T10:44:16Z

@tiwaria1 please don't forget to add a test :)

tiwaria1 · 2023-01-26T12:44:30Z

@tiwaria1 please don't forget to add a test :)

Added one. I did not run it locally though. Is there any automatic git CI testing that will run it for me on this PR?

MrSidims · 2023-01-26T12:48:07Z

@tiwaria1 please don't forget to add a test :)
Added one. I did not run it locally though. Is there any automatic git CI testing that will run it for me on this PR?

Yet here is, but I expect it to be broken right now due to LLVM upgrade to 17.0.0 . So I would suggest running tests locally (it won't faster the merge, since we would need to wait for the fixed CI, but still help with finding problems if any). To run test you should add -DLLVM_SPIRV_INCLUDE_TESTS=ON to cmake and run make test -j...

MrSidims

To incorporate changes from SPIR-V headers please update spirv-headers-tag.conf

lib/SPIRV/libSPIRV/spirv_internal.hpp

lib/SPIRV/SPIRVWriter.cpp

tiwaria1 · 2023-01-27T10:56:28Z

To incorporate changes from SPIR-V headers please update spirv-headers-tag.conf

I can only do that once the SPIRV-Headers PR is merged right?

MrSidims · 2023-01-27T12:21:36Z

To incorporate changes from SPIR-V headers please update spirv-headers-tag.conf

I can only do that once the SPIRV-Headers PR is merged right?

Ah, sorry, I've seen notification, that the PR got closed and assumed, that is was merged.

MrSidims

LGTM, just waiting for the headers be merged
Thanks!

tiwaria1 · 2023-03-01T19:22:16Z

Hey @MrSidims the headers change has been merged. Has the CI been fixed yet?

MrSidims · 2023-03-03T13:27:24Z

@tiwaria1 please take a look at the test failure

tiwaria1 · 2023-03-06T15:22:47Z

Having trouble building the translator locally, I did the following (llvm in-tree build steps):

git clone https://github.com/llvm/llvm-project.git
cd llvm-project/llvm/projects/
git clone https://github.com/tiwaria1/SPIRV-LLVM-Translator.git
cd SPIRV-LLVM-Translator/
# checkout my branch
mkdir build
cd build/
cmake ../llvm -DLLVM_ENABLE_PROJECTS="clang" -DCMAKE_BUILD_TYPE=Debug
make llvm-spirv -j`nproc`

and got the error:

/p/psg/swip/w/tiwaria1/ipauth/tiwaria1_specs/llvm-project/llvm/projects/SPIRV-LLVM-Translator/lib/SPIRV/SPIRVLowerConstExpr.cpp:48:10: fatal error: llvm/ADT/Triple.h: No such file or directory
 #include "llvm/ADT/Triple.h"
          ^~~~~~~~~~~~~~~~~~~
compilation terminated.
projects/SPIRV-LLVM-Translator/lib/SPIRV/CMakeFiles/LLVMSPIRVLib.dir/build.make:257: recipe for target 'projects/SPIRV-LLVM-Translator/lib/SPIRV/CMakeFiles/LLVMSPIRVLib.dir/SPIRVLowerConstExpr.cpp.o' failed

tiwaria1 · 2023-03-06T18:55:45Z

Not sure what's up with clang-format check, it says:

Run cat diff-to-inspect.txt | /usr/share/clang/clang-format-17/clang-format-diff.py -p1 > clang-format.patch
Traceback (most recent call last):
  File "/usr/share/clang/clang-format-17/clang-format-diff.py", line 177, in <module>
    main()
  File "/usr/share/clang/clang-format-17/clang-format-diff.py", line [13](https://github.com/KhronosGroup/SPIRV-LLVM-Translator/actions/runs/4345812544/jobs/7591095087#step:6:14)5, in main
    process_subprocess_result(proc, args)
  File "/usr/share/clang/clang-format-[17](https://github.com/KhronosGroup/SPIRV-LLVM-Translator/actions/runs/4345812544/jobs/7591095087#step:6:18)/clang-format-diff.py", line 42, in process_subprocess_result
    with open(filename) as f:
NameError: name 'filename' is not defined
Error: Process completed with exit code 1.

MrSidims · 2023-03-06T19:16:18Z

Not sure what's up with clang-format check, it says:

I'll take a look at clang-format check, you may ignore it
Please take a look at the failed test:
Failed Tests (1):
LLVM_SPIRV :: extensions/INTEL/SPV_INTEL_kernel_attributes/register_map_interface_attribute.ll

/home/runner/work/SPIRV-LLVM-Translator/SPIRV-LLVM-Translator/SPIRV-LLVM-Translator/test/extensions/INTEL/SPV_INTEL_kernel_attributes/register_map_interface_attribute.ll:11:16: error: CHECK-SPIRV: expected string not found in input
; CHECK-SPIRV: Capability FPGAKernelAttributesv2INTEL
               ^
<stdin>:5:39: note: scanning from here
2 Capability FPGAKernelAttributesINTEL 
                                      ^
<stdin>:7:9: note: possible intended match here
8 Extension "SPV_INTEL_kernel_attributes"

…CHECK-DAG

tiwaria1 · 2023-03-07T18:13:07Z

I'll take a look at clang-format check, you may ignore it Please take a look at the failed test: Failed Tests (1): LLVM_SPIRV :: extensions/INTEL/SPV_INTEL_kernel_attributes/register_map_interface_attribute.ll

It was the missing entry in SPIRVNameMapEnum.h that was causing this. Fixed.

tiwaria1 · 2023-03-08T14:19:37Z

What do I do about the clang-format error? Looks like it is some sort of script issue?

…properties, fix RegisterMap sycl-post-link (#10406) Add defaults for template parameters for streaming_interface and register_map_interface in fpga_kernel_properties.hpp since current usage gets too verbose, and defaults are allowed and implementation defined. Register map interface also does not match SPIRV expected interface [spec](https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/INTEL/SPV_INTEL_kernel_attributes.asciidoc) and [implementation](KhronosGroup/SPIRV-LLVM-Translator#1774), so update sycl-post-link.

…properties, fix RegisterMap sycl-post-link (intel#10406) Add defaults for template parameters for streaming_interface and register_map_interface in fpga_kernel_properties.hpp since current usage gets too verbose, and defaults are allowed and implementation defined. Register map interface also does not match SPIRV expected interface [spec](https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/INTEL/SPV_INTEL_kernel_attributes.asciidoc) and [implementation](KhronosGroup/SPIRV-LLVM-Translator#1774), so update sycl-post-link.

first draft of RegisterMapInterfaceINTEL exec mode

edb8c4c

MrSidims reviewed Dec 9, 2022

View reviewed changes

lib/SPIRV/SPIRVWriter.cpp Outdated Show resolved Hide resolved

tiwaria1 added 3 commits January 26, 2023 04:03

merge main, move registermap out of internal namespace

fd2f1b6

move out of internal space

6a20265

add lit test

23293ca

MrSidims reviewed Jan 26, 2023

View reviewed changes

lib/SPIRV/libSPIRV/spirv_internal.hpp Outdated Show resolved Hide resolved

lib/SPIRV/SPIRVWriter.cpp Outdated Show resolved Hide resolved

MrSidims reviewed Jan 26, 2023

View reviewed changes

lib/SPIRV/SPIRVWriter.cpp Outdated Show resolved Hide resolved

restore empty line

673c658

tiwaria1 added 2 commits January 27, 2023 08:03

correct implict declaration of capability

5c85440

update comment

37cc1f6

MrSidims reviewed Jan 27, 2023

View reviewed changes

tiwaria1 added 2 commits March 1, 2023 10:44

change parameter name to align with spec

01c3a5f

update spirv-headers-tag

0eaf0ed

MrSidims approved these changes Mar 3, 2023

View reviewed changes

tiwaria1 added 2 commits March 6, 2023 07:32

Merge branch 'KhronosGroup:main' into register_map_interface

7355418

run clang-format

a44ca18

replace CHECK with CHECK-DAG for lit test

10eb8bf

add capabilit to SPIRVNameMapEnum; reset lit to use CHECK instead of …

523e24e

…CHECK-DAG

MrSidims merged commit a9f4f25 into KhronosGroup:main Mar 8, 2023

bowenxue-intel mentioned this pull request Jul 14, 2023

[SYCL][FPGA] Add defaults for streaming and register_map fpga_kernel_properties, fix RegisterMap sycl-post-link intel/llvm#10406

Merged

Implement RegisterMapInterfaceINTEL execution mode #1774

Implement RegisterMapInterfaceINTEL execution mode #1774

Uh oh!

Conversation

tiwaria1 commented Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

NOTE

Uh oh!

CLAassistant commented Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tiwaria1 commented Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

MrSidims commented Dec 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MrSidims commented Dec 9, 2022

Uh oh!

tiwaria1 commented Jan 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MrSidims commented Jan 26, 2023

Uh oh!

MrSidims left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tiwaria1 commented Jan 27, 2023

Uh oh!

MrSidims commented Jan 27, 2023

Uh oh!

MrSidims left a comment

Choose a reason for hiding this comment

Uh oh!

tiwaria1 commented Mar 1, 2023

Uh oh!

MrSidims commented Mar 3, 2023

Uh oh!

tiwaria1 commented Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tiwaria1 commented Mar 6, 2023

Uh oh!

MrSidims commented Mar 6, 2023

Uh oh!

tiwaria1 commented Mar 7, 2023

Uh oh!

tiwaria1 commented Mar 8, 2023

Uh oh!

Uh oh!

tiwaria1 commented Dec 8, 2022 •

edited

Loading

CLAassistant commented Dec 8, 2022 •

edited

Loading

tiwaria1 commented Dec 8, 2022 •

edited

Loading

MrSidims commented Dec 9, 2022 •

edited

Loading

tiwaria1 commented Jan 26, 2023 •

edited

Loading

tiwaria1 commented Mar 6, 2023 •

edited

Loading