Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

target/riscv: implement abstract command cache #1235

Open
wants to merge 1 commit into
base: riscv
Choose a base branch
from

Conversation

fk-sc
Copy link
Contributor

@fk-sc fk-sc commented Mar 4, 2025

Implemented cache of unsupported abstract commands. It's purpose is to replace set of caching variables to one. So this commit provides one ac_not_supported_cache instead of abstract_read_csr_supported,
abstract_write_csr_supported, abstract_read_fpr_supported, abstract_write_fpr_supported, has_aampostincrement.

Dropped check for buggy aampostincrement

Fixes #1232

Change-Id: I75cae1481841f2cd0393d6cc80f0d913fbe34294

@fk-sc fk-sc force-pushed the fk-sc/abstract-cmd-cache branch from 5b975e0 to cc7e5e3 Compare March 4, 2025 08:50
@fk-sc
Copy link
Contributor Author

fk-sc commented Mar 4, 2025

@JanMatCodasip, @MarekVCodasip, could you please take a look?

Copy link
Collaborator

@JanMatCodasip JanMatCodasip left a comment

Choose a reason for hiding this comment

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

Thank you for this improvement. Overall, the code looks all right. I have found several things to address, though. Could you please check my comments.

@fk-sc fk-sc force-pushed the fk-sc/abstract-cmd-cache branch 2 times, most recently from 13ade68 to 0267349 Compare March 15, 2025 12:54
@fk-sc fk-sc requested a review from JanMatCodasip March 15, 2025 12:56
@fk-sc fk-sc force-pushed the fk-sc/abstract-cmd-cache branch from 0267349 to 272c514 Compare March 18, 2025 12:24
Copy link
Collaborator

@JanMatCodasip JanMatCodasip left a comment

Choose a reason for hiding this comment

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

@fk-sc thank you for addressing the findings so far.

I came across few more things - if you can please look at them.

@fk-sc fk-sc force-pushed the fk-sc/abstract-cmd-cache branch from 272c514 to 4e0f011 Compare March 19, 2025 10:54
@fk-sc fk-sc requested a review from JanMatCodasip March 19, 2025 11:03
@fk-sc fk-sc force-pushed the fk-sc/abstract-cmd-cache branch 2 times, most recently from 946330b to ff8bb84 Compare March 19, 2025 12:27
Copy link
Collaborator

@JanMatCodasip JanMatCodasip left a comment

Choose a reason for hiding this comment

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

Overall this MR looks good. I will do one last proper review tomorrow.

@fk-sc fk-sc force-pushed the fk-sc/abstract-cmd-cache branch from ff8bb84 to 6bd4a06 Compare March 19, 2025 15:49
@fk-sc fk-sc requested a review from JanMatCodasip March 19, 2025 15:51
Copy link
Collaborator

@JanMatCodasip JanMatCodasip left a comment

Choose a reason for hiding this comment

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

Overall, this looks fine. I am sending my last batch of comments, mostly it is minor coding stuff.

I do not expect to have any more comments than that.

@fk-sc fk-sc force-pushed the fk-sc/abstract-cmd-cache branch 3 times, most recently from e7ae250 to a7b5f42 Compare March 24, 2025 08:52
JanMatCodasip
JanMatCodasip previously approved these changes Mar 24, 2025
Copy link
Collaborator

@JanMatCodasip JanMatCodasip left a comment

Choose a reason for hiding this comment

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

LGTM, thank you.

Implemented cache of unsupported abstract commands.
It's purpose is to replace set of caching variables to one.
So this commit provides one ac_not_supported_cache
instead of abstract_read_csr_supported,
abstract_write_csr_supported, abstract_read_fpr_supported,
abstract_write_fpr_supported, has_aampostincrement.

Dropped check for buggy aampostincrement

Fixes riscv-collab#1232

Change-Id: I75cae1481841f2cd0393d6cc80f0d913fbe34294
Signed-off-by: Farid Khaydari <[email protected]>
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.

Implement generic abstract command support cache
2 participants