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

Fix: Implement direct C_ADDIW clause and remove redundant operations #38

Closed
wants to merge 1 commit into from

Conversation

harshagr70
Copy link

Description

This PR addresses issue https://github.com/ThinkOpenly/sail/issues/32 by substituting the indirect reference in the execute function clause for the C_ADDIW pattern with the actual implementation code.

Changes Made

  • Directly included the functionality of the ADDIW clause within the C_ADDIW clause.
  • Removed redundant sign_extend calls to enhance code efficiency.

Motivation and Context

The original implementation used an indirect reference to ADDIW within the C_ADDIW clause, leading to redundant operations and potential inefficiencies. By incorporating the code directly, we ensure that the C_ADDIW clause executes more efficiently and correctly.

Related Issues

Fixes

: sail#32

@ThinkOpenly
Copy link
Owner

Thank you for taking on the effort to submit this proposal!

There is a reason the issue was opened in the https://github.com/ThinkOpenly/sail repository, and not this repository. Looking at the referenced issue, though, it does not explain the expectations well. (My bad.)

In essence, the Sail code in question is correct, and condense. Rather than duplicating code, it calls out to the (non-compressed) base instruction's implementation.

The preferred solution[1] would be to emit the (duplicated) code in the JSON produced by the Sail JSON backend, without changing the Sail code at all. The source for the backend is in that "sail" repository.

Sorry for the confusion.

[1] It could be that the code emitted in JSON currently is just fine, and the burden of linking the indirect call to the actual code should be left to downstream users of the JSON, like that website referenced in ThinkOpenly/sail#32 (comment).

@harshagr70
Copy link
Author

@ThinkOpenly this makes so much sense , thanks for the guidance i will check this and update it accordingly !!

@harshagr70 harshagr70 closed this Aug 20, 2024
@harshagr70
Copy link
Author

@ThinkOpenly i am closing this pull request as it do not address the required changes .

@harshagr70 harshagr70 deleted the feature branch November 24, 2024 05:47
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