spcl / dace

DaCe - Data Centric Parallel Programming
http://dace.is/fast
BSD 3-Clause "New" or "Revised" License
491 stars 127 forks source link

Xilinx FPGA hardware compilation fails because dace generates invalid kernel names (in projects with hierarchical folder structure) that are more than 64 characters long #1068

Open krishnakumarg1984 opened 2 years ago

krishnakumarg1984 commented 2 years ago

Describe the bug It is common practice for python projects to use a nested directory hierarchy of files for modular organisation and grouping of related code and data files.

When a kernel is defined as a dace-annotated python function embedded in a nested project file, the corresponding kernel name is excessively long. Dace merely concatenates the nested folder and function names together to arrive at the kernel name string which violates the limit of 64 characters for Xilinix FPGA toolchain. Thus, the compilation errors out.

This occurs only when compiling for hardware and not when setting the mode to simulation.

To Reproduce Consider the folder structure:

.
├── my_custom_project
│   ├── input_data
│   │   └── initguess.py
│   └── custom_algorithm
│       ├── algo_gen_sdfg.py
│       └── custom_kernel.py
├── setup.py
└── usercode_examples
    └── simulation.py

In this case, we are making a local pip installable package called my_custom_project.

If a dace kernel having a function name of custom_iteration() is defined in custom_kernel.py, Dace just concatenates the kernel name to produce an excessively long kernel name i.e. something like my_custom_project_custom_algorithm_custom_kernel_custom_iteration_0_0_1().

This fails when compiling for Xilinx FPGA hardware since kernel names can be utmost 64 characters in length.

Expected behavior A suitably short kernel name is generated by dace that is at most 64 characters in length to satisfy the Vitis compiler.

Additional context Only occurs when dace mode is hardware. Works well with simulation mode.

JamieJQuinn commented 2 years ago

Seems like the relevant piece is name() in parser. @TizianoDeMatteis @definelicht any way we could provide a custom kernel name when defining the program to avoid the longer automatic one?

tbennun commented 2 years ago

Funnily enough, this is tied to our earliest open issue: #28

I think you can change the name of the SDFG though with sdfg.name = '...'

JamieJQuinn commented 2 years ago

Ahh yes, I didn't see name in the SDFG code. Thanks @tbennun.

To warn the user of this, where would be the best place to validate the length of the filename before the HLS code is written?

tbennun commented 2 years ago

It's a Xilinx-specific issue, so it would be safest to raise an exception (ValueError) in the code generator. For example, here: https://github.com/spcl/dace/blob/master/dace/codegen/targets/xilinx.py#L541

Then the name should be there mentioning the 64-character limitation, and recommend changing the sdfg.name and map.label to make it shorter.

Alternatively, one could check the schedule here in validation: https://github.com/spcl/dace/blob/master/dace/sdfg/nodes.py#L856 but that would not be advised because the toolchain is configurable.

Another solution we've thought of before (this is not the first time we encounter this limitation) is to trim the kernel name and leave the end of it (kernel_name[-63:]), but that leaves unintelligible names and opens the possibility of unwanted name clashes which would be bad in ways we cannot anticipate. Of course the real solution would be to ask the Xilinx toolchain developers to remove this limitation.

krishnakumarg1984 commented 2 years ago

Another solution we've thought of before (this is not the first time we encounter this limitation) is to trim the kernel name and leave the end of it (kernel_name[-63:]), but that leaves unintelligible names and opens the possibility of unwanted name clashes which would be bad in ways we cannot anticipate.

Agreed. But can dace perhaps consider naming the kernel as just the name of the decorated function, i.e. without the hierarchical folder concatenation? This would be the easiest solution. It is very common in python projects to have some kind of folder hierarchy, and while this is very much an upstream Xilinx issue, it would be nice to keep the kernel names short and readable regardless.

JamieJQuinn commented 2 years ago

@krishnakumarg1984 I agree it would be nice but it opens up the possibility of name clashes, e.g. if you had an add kernel in two separate modules, they could both produce generated code with the same function signature. Telling the user the code won't compile for the very specific Xillinx hardware case doesn't introduce a clash possibility for all users while still allowing that user to set a manual kernel name.

@tbennun okay, we'll look into adding a warning, cheers!

tbennun commented 2 years ago

@krishnakumarg1984 at first, we only had the function names, but exactly as @JamieJQuinn said, once we started compiling multiple modules the names started clashing very quickly. It's a necessity to maintain correctness for other projects and platforms.