Skip to content

SelectToken#1577

Open
ollycassidy13 wants to merge 9 commits into
Xilinx:devfrom
ollycassidy13:feature/SelectToken
Open

SelectToken#1577
ollycassidy13 wants to merge 9 commits into
Xilinx:devfrom
ollycassidy13:feature/SelectToken

Conversation

@ollycassidy13

Copy link
Copy Markdown

Adds the SelectToken fpgadataflow layer and RTL implementation, converting supported Gather patterns into a hardware SelectToken op for selecting one token from a token sequence.

@STFleming STFleming left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work @ollycassidy13, this looks like a great PR.

Only main issue I could see was license header consistency along with the resource estimate functions looking a bit off. I think we should probably increase the test coverage also with more parameters being changed, but let's discuss that.

@@ -0,0 +1,272 @@
# Copyright (C) 2026, Advanced Micro Devices, Inc.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use new lic header

Copyright Advanced Micro Devices, Inc.
SPDX-License-Identifier: BSD-3-Clause

@@ -0,0 +1,77 @@
/******************************************************************************

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the new lic header

Copyright Advanced Micro Devices, Inc.
SPDX-License-Identifier: BSD-3-Clause

@@ -0,0 +1,132 @@
# Copyright (C) 2026, Advanced Micro Devices, Inc.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the new lic header

Copyright Advanced Micro Devices, Inc.
SPDX-License-Identifier: BSD-3-Clause

@@ -0,0 +1,155 @@
# Copyright (C) 2026, Advanced Micro Devices, Inc.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use the new lic

Copyright Advanced Micro Devices, Inc.
SPDX-License-Identifier: BSD-3-Clause

self.set_nodeattr("outputDataType", idt.name)
model.set_tensor_datatype(node.output[0], idt)

def verify_node(self):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just use the base class implementation? Or override with some checks.

def bram_estimation(self):
return 0

def lut_estimation(self):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this 200? This being a magic flat constant feels off :).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should widen the parameter matrix here to test more parameter configurations?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants