Skip to content

Conversation

@ezelioli
Copy link
Collaborator

No description provided.

@ezelioli ezelioli requested review from alex96295 and bluewww March 25, 2025 10:11
@ezelioli ezelioli self-assigned this Mar 25, 2025
@ezelioli ezelioli marked this pull request as ready for review March 25, 2025 23:06
if (SSCLIC) begin
reg_all_int_req.addr = reg_req_i.addr - SCLICINT_START;
if (intmode[reg_all_int_req.addr[15:2]] <= S_MODE) begin
addr_tmp = reg_req_i.addr[ADDR_W-1:0] - SCLICINT_START;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you remember why you had to introduce addr_tmp? Sim bug?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure anymore unfortunately. Looking at the code it could be also simply for readability, but I am open to comments on that.

@ezelioli
Copy link
Collaborator Author

ezelioli commented Apr 3, 2025

@bluewww @alex96295 I found a new problem when synthesizing this version of the CLIC with a different synthesis compiler. It seems that the problem is related to the parametric defines, which are not seen as constants in the evaluation of the if else within the for loop in line 669. The synthesis tools requires the LHS of the comparison within the if to be a constant, breaking the elaboration.

I pushed a fix which is not really elegant but seems to work for all synthesis tools tried and does not require unrolling the entire for loop, which would be way worse. Ideally I think we could move some of the localparams in clic.sv to clic_pkg.sv, however due to the fact that most of them rely on parameters passed to the clic top module this is not possible at the moment. Suggestions on style changes are welcome :)

return (dividend + divisor - 1) / divisor;
endfunction

function automatic int unsigned rounddown(int unsigned value, int unsigned alignment);
Copy link
Contributor

Choose a reason for hiding this comment

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

ideally better to add this to common cells if it is of general use, can be done later

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will do

Copy link
Contributor

@alex96295 alex96295 left a comment

Choose a reason for hiding this comment

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

LGTM

@alex96295
Copy link
Contributor

@bluewww @alex96295 I found a new problem when synthesizing this version of the CLIC with a different synthesis compiler. It seems that the problem is related to the parametric defines, which are not seen as constants in the evaluation of the if else within the for loop in line 669. The synthesis tools requires the LHS of the comparison within the if to be a constant, breaking the elaboration.

I pushed a fix which is not really elegant but seems to work for all synthesis tools tried and does not require unrolling the entire for loop, which would be way worse. Ideally I think we could move some of the localparams in clic.sv to clic_pkg.sv, however due to the fact that most of them rely on parameters passed to the clic top module this is not possible at the moment. Suggestions on style changes are welcome :)

I think for the time being we can leave it like this. I remember that Synopsys DC was able to digest the version you just modified (what was not able to digest was the original version with a case switch). For the sake of the projects depending on this IPs, I propose we merge this and we open an issue right now as a reminder to improve this logic. It can also be that meanwhile the tools that do not support this (Yosys?) will be improved as well

Co-authored by: bluew <bluewww@users.noreply.github.com>
@ezelioli
Copy link
Collaborator Author

ezelioli commented Apr 8, 2025

Fixed history. Ready to merge.

@alex96295 alex96295 changed the title Enable CLIC virtualization treewide: Enable CLIC virtualization Apr 8, 2025
@alex96295 alex96295 merged commit 72472df into master Apr 8, 2025
7 checks passed
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.

4 participants