Skip to content

Conversation

@marcelkant
Copy link

This PR introduces initial support for handling arbitrary matrix shapes by adjusting both the Python model and the Softmax hardware module.

Changes

Python Model:

  • Modified the golden model for arbitrary shapes by padding the inputs with zeros as needed.
  • Updated to selectively ignore padded input values during the softmax operation

Current Limitations

  • The HWPE version does not yet work correctly. There is most likely a bug

ToDo

  • Fix HWPE tests with padded values

@Xeratec Xeratec requested review from Xeratec and gamzeisl January 24, 2025 18:54
@Xeratec Xeratec added the enhancement New feature or request label Jan 24, 2025
Copy link
Collaborator

@gamzeisl gamzeisl left a comment

Choose a reason for hiding this comment

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

Overall, looks good! Minor changes are needed though.

  • Fix CI.
  • Fix HWPE and extend CI tests.

- python testGenerator.py -H 1 -S 64 -E 64 -P 64 -F 64 --activation gelu
- python testGenerator.py -H 1 -S 128 -E 192 -P 256 -F 256 --activation gelu
- python testGenerator.py -H 1 -S 192 -E 256 -P 128 -F 128 --activation relu
- python testGenerator.py -H 1 -S 64 -E 64 -P 64 -F 64 --activation gelu --skip-vector-validation
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is disabling validation (--skip-vector-validation) a good idea for all? I would say to enable it for a subset of tests

"-S${input:seq_len}",
"-E${input:emb_len}",
"-P${input:prj_len}",
"--no-bias"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be left out

Comment on lines +116 to +119
# assert (self.S % self.ITA_M == 0), "Sequence length must be divisible by ITA_M"
# assert (self.P % self.ITA_M == 0), "Projection space must be divisible by ITA_M"
# assert (self.E % self.ITA_M == 0), "Embedding size must be divisible by ITA_M"
# assert (self.F % self.ITA_M == 0), "Feedforward size must be divisible by ITA_M"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove

Comment on lines +361 to +363
# print(f"qk: {qk.shape}")
# print(f"qk: {weight.shape}")

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why these are commented?

Comment on lines +378 to +390
# fig, ax = plt.subplots(1, 2) # Create a figure with two subplots
# im0 = ax[0].imshow(Input, cmap='viridis')
# im1 = ax[1].imshow(np.squeeze(weight, axis=0))

# # Add colorbars for each image if needed
# fig.colorbar(im0, ax=ax[0])
# fig.colorbar(im1, ax=ax[1])

# # Set titles for each subplot
# ax[0].set_title("Inputs")
# ax[1].set_title("Weights")

plt.show()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why commented?

self.requant_add_ffn[0])
self.FFp_requant = self.apply_activation(self.FFp_requant, self.activation)

Copy link
Collaborator

Choose a reason for hiding this comment

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

can be omitted

Comment on lines +48 to +51
typedef logic [WO-WI*2-2:0] seq_length_t;
typedef logic [WO-WI*2-2:0] proj_space_t;
typedef logic [WO-WI*2-2:0] embed_size_t;
typedef logic [WO-WI*2-2:0] ff_size_t;
Copy link
Collaborator

Choose a reason for hiding this comment

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

WO-WI*2-2 is unclear, why the bit width is set to this?

Copy link
Author

Choose a reason for hiding this comment

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

I also do not know why this bit width was defined this way. It was already used for the S, E, and P dimensions, so I used it for the F dimension as well. Should I change it to a constant?

Comment on lines +36 to +41
// always_comb begin
// requant_mult = ctrl_i.eps_mult[step_q4];
// requant_shift = ctrl_i.right_shift[step_q4];
// requant_add = ctrl_i.add[step];
// end

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove

marcelkant and others added 2 commits February 8, 2025 15:06
Typo fix

Co-authored-by: Gamze İslamoğlu <54476562+gamzeisl@users.noreply.github.com>
Co-authored-by: Gamze İslamoğlu <54476562+gamzeisl@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants