Where#1579
Conversation
STFleming
left a comment
STFleming
left a comment
There was a problem hiding this comment.
Thanks Olly, this is a great PR!
I have a few comments / suggestions.
| @@ -0,0 +1,159 @@ | |||
| # Copyright (C) 2026, Advanced Micro Devices, Inc. | |||
There was a problem hiding this comment.
Please replace the license header with
Copyright Advanced Micro Devices, Inc.
SPDX-License-Identifier: BSD-3-Clause
|
|
||
|
|
||
| class Where_rtl(Where, RTLBackend): | ||
| """RTL implementation of Where.""" |
There was a problem hiding this comment.
Maybe add a little more info here, or say the ONNX operator Where maybe?
| template_path = rtlsrc + "/where_template.v" | ||
| with open(template_path, "r") as f: | ||
| template = f.read() | ||
| core_template_path = rtlsrc + "/where_core_template.sv" |
There was a problem hiding this comment.
I'm not quite sure why we have two templates here. Usually what happens is we have an <op name>_axi.sv and a <op name>_template.v and inside the <op name>_axi module is instantiated with parameters being passed through.
Maybe there is a good reason for having this additional template complexity logic, but it's not instantly obvious to me.
There was a problem hiding this comment.
Ahh is this because you want to do packed array literals and you need to parameterise? Maybe make this clear in a comment.
| @@ -0,0 +1,238 @@ | |||
| # Copyright (C) 2026, Advanced Micro Devices, Inc. | |||
There was a problem hiding this comment.
Please replace the lic header with the newer version
Copyright Advanced Micro Devices, Inc.
SPDX-License-Identifier: BSD-3-Clause
| {"auto", "block", "distributed", "ultra"}, | ||
| ), | ||
| "inFIFODepths": ("ints", False, [2, 2, 2]), | ||
| "outFIFODepths": ("ints", False, [2]), |
There was a problem hiding this comment.
I think outFIFODepths is the same as the base class so could be dropped from here. I might add a comment to inFIFODepths for the reason why it is being overridden.
| graph_modified = False | ||
| for node in graph.node: | ||
| node_ind += 1 | ||
| if node.op_type != "Where" or node.domain not in ["", "ai.onnx"]: |
There was a problem hiding this comment.
Does ai.onnx appear in practice, I thought the empty domain would be enough?
@auphelia, is it an issue that the fpgadataflow op has the same name as the ONNX level op?
| model.set_tensor_datatype(cond_name, DataType["BINARY"]) | ||
| cond_dt = DataType["BINARY"] | ||
| if cond_dt != DataType["BINARY"]: | ||
| continue |
There was a problem hiding this comment.
For some of these continue breaks I feel like we should add some Warnings for why the infer was aborted. Like what we have for some of the other layers example
| @@ -0,0 +1,615 @@ | |||
| # Copyright (C) 2026, Advanced Micro Devices, Inc. | |||
There was a problem hiding this comment.
Please use the new lic header
Copyright Advanced Micro Devices, Inc.
SPDX-License-Identifier: BSD-3-Clause
There was a problem hiding this comment.
A general comment on the testing would be to utilise test parameterisation more rather than having duplicated code and avoiding "same body, different shape tuple" code duplication.
| assert ret["LUT"] > 0 | ||
| assert ret["FF"] > 0 | ||
| assert ret["DSP"] == 0 | ||
| assert ret.get("BRAM_18K", 0) == 0 |
There was a problem hiding this comment.
Isn't this a function of how big the problem size is and we might start seeing BRAMs being used for larger shapes, or will the input_gen.sv always use LUTRAM in these cases? It looks like it's RAM_STYLE is being set to auto so I would expect that this might happen.
We might want to improve this test and test against the estimates resource usage for particular problem sizes. @auphelia or might doing this too much slow down testing time?
2 participants
No description provided.