Extract FPGA code generation to separate repository #2252
Conversation
This reverts commit 75fde04.
|
cscs-ci run |
|
|
||
|
|
||
| def cpp_offset_expr(d: data.Data, subset_in: subsets.Subset, offset=None, packed_veclen=1, indices=None): | ||
| def cpp_offset_expr(d: data.Data, |
There was a problem hiding this comment.
The main reason cpp_offset_expr needs the codegen as input is that the FPGA backend handles HBM-distributed/Bank-distributed arrays at the codegen level, not at the SDFG level, correct?
I fear that providing the codegen might encourage hacks in the future. I am not sure, but would making offset expression a function that can be overridden by the codegen discourage this possibility (probably it does not change anything)? (I do not have a solution for how to fix it on the FPGA side)
There was a problem hiding this comment.
Similar to framecode.ptr and cpp.cpp_ptr_expr -> framecode.offset and cpp.cpp_offset_expr?
There was a problem hiding this comment.
This is true and the intention is to remove some of those arguments in a following PR. However, we have to keep them to support the FPGA codegen at least in the next prerelease (2.0.0-alpha2).
There was a problem hiding this comment.
for cpp_offset_expr, the codegen is needed so that codegen.ptr can be called. This allows C++ code generators to extend the behavior of ptr while still using the CPU and frame code generators.
Of course, one could clean this behavior up in a redo of the code generation infrastructure, but this enables flexibility IMO.
There was a problem hiding this comment.
I support reducing the arguments. I do not think the current arglist is an issue.
| def sizes(self): | ||
| return [d.name if isinstance(d, symbolic.symbol) else str(d) for d in self.shape] | ||
|
|
||
| def size_string(self): |
There was a problem hiding this comment.
I think this function can be left in the main repo. I think it could be useful when generating allocations (for example, when we implement explicit allocation support or new backends). Is this function available somewhere else?
There was a problem hiding this comment.
I disagree. It is creating a circular import to dace.codegen and cppunparse in general. There is symstr in dace.symbolic that should be used instead, and this is just a duplicate way to call symstr(desc.total_size, cpp=True)
There was a problem hiding this comment.
True, having this in symbolic.py is definitely better.
| } | ||
|
|
||
| # Translation of types to OpenCL types | ||
| _OCL_TYPES = { |
There was a problem hiding this comment.
Would it make sense to keep a dictionary that maps numpy types to C types and encourage its use for code generators that need to generate C code? Would it fall into the you-arent-gonna-need-it category?
There was a problem hiding this comment.
C types I am not sure. Code generators can provide that mapping themselves IMO. It was also creating diverging types in certain code generators and causing C++ code to be emitted in OpenCL (e.g., references, dace::vec), so it is better this way. It enforces code generators to be more strict about their own types. I did this now in the Intel FPGA backend and it helps clean up the code.
There was a problem hiding this comment.
That is also possible. Needing to generate C code also comes with the issue that we can't most of the dace headers, so a backend that needs to generate C code can also create its own type mapping (and it is better not to load the dtype file definitely with it). It falsely gives the idea that DaCe can switch to C codegen easily.
|
Do we need to update the copyright year on the files that have been modified from 2019-XXX to 2019-2026? Some files have been modified but the copyright year was not updated |
dace/runtime/include/dace/vector.h
Outdated
| #include "xilinx/vec.h" | ||
| #else // Don't include this file if building for Xilinx | ||
| #include <dace_fpga/xilinx/vec.h> | ||
| // Don't include this file if building for Xilinx |
There was a problem hiding this comment.
Shouldn't this comment say: // Don't include this file if not building for Xilinx
because the header is included if XILINX is defined, and not included if it is not defined
There was a problem hiding this comment.
Next PR will clean this up. There was no way to disentangle that dependency unfortunately :(
There was a problem hiding this comment.
I will edit the comment though
|
Other than the signature of the |
dace/sdfg/type_inference.py
Outdated
| @@ -1 +1 @@ | |||
| # Copyright 2019-2021 ETH Zurich and the DaCe authors. All rights reserved. | |||
There was a problem hiding this comment.
| # Copyright 2019-2026 ETH Zurich and the DaCe authors. All rights reserved. |
dace/sdfg/validation.py
Outdated
| @@ -1 +1 @@ | |||
| # Copyright 2019-2024 ETH Zurich and the DaCe authors. All rights reserved. | |||
There was a problem hiding this comment.
| # Copyright 2019-2026 ETH Zurich and the DaCe authors. All rights reserved. |
| @@ -1 +1 @@ | |||
| # Copyright 2019-2021 ETH Zurich and the DaCe authors. All rights reserved. | |||
There was a problem hiding this comment.
| # Copyright 2019-2026 ETH Zurich and the DaCe authors. All rights reserved. |
dace/config.py
Outdated
| @@ -1 +1 @@ | |||
| # Copyright 2019-2022 ETH Zurich and the DaCe authors. All rights reserved. | |||
There was a problem hiding this comment.
| # Copyright 2019-2026 ETH Zurich and the DaCe authors. All rights reserved. |
dace/dtypes.py
Outdated
| @@ -1 +1 @@ | |||
| # Copyright 2019-2023 ETH Zurich and the DaCe authors. All rights reserved. | |||
There was a problem hiding this comment.
| # Copyright 2019-2026 ETH Zurich and the DaCe authors. All rights reserved. |
- [x] Remove ArrayInterface - [x] Remove other FPGA-related enumeration entries - [x] Mitigate extraneous arguments in `dace.codegen.targets.cpp` - [x] Remove runtime includes and references to DaCe-FPGA macros - [x] Address TODOs from #2252
The DaCe FPGA backend will be supported out-of-tree. This PR extracts the capabilities to a separate repository: https://github.com/spcl/dace-fpga
This PR also reduces circular imports and encourages more object-oriented design for code generation.