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

[JSON]: Handling Optional Operands #17

Merged
merged 1 commit into from
May 23, 2024
Merged

Conversation

snapdgn
Copy link

@snapdgn snapdgn commented Apr 11, 2024

No description provided.

@snapdgn snapdgn changed the title [JSON]: fixes: #2, added optional operand field to operands [JSON]: Handling Optional Operands Apr 11, 2024
@snapdgn
Copy link
Author

snapdgn commented Apr 11, 2024

[WIP]: This needs more work. The logic for extracting operands is not sound currently, as we need to extract it from the actual maybe_vmask. Also syntax is not updated reflecting the presence / absence of the optional operands.

@snapdgn
Copy link
Author

snapdgn commented Apr 26, 2024

This currently extends the operands with an optional boolean field:
Result it something like this:

{
  "mnemonic": "vsetivli",
  "name": "TBD",
  "operands": [ { "name": "ma", "type": "bits(1)", "optional": true },{ "name": "ta", "type": "bits(1)", "optional": true },{ "name": "sew", "type": "bits(3)", "optional": false },{ "name": "lmul", "type": "bits(3)", "optional": true },{ "name": "uimm", "type": "regidx", "optional": false },{ "name": "rd", "type": "regidx", "optional": false } ],
  "syntax": "rd,uimm,sew_flag(sew)maybe_lmul_flag(lmul)maybe_ta_flag(ta)maybe_ma_flag(ma)",
...
}

Copy link
Owner

@ThinkOpenly ThinkOpenly left a comment

Choose a reason for hiding this comment

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

I did a cursory review as I head out the door, and will revisit this tomorrow. I have left some comments that could be addressed in the meantime.

src/sail_json_backend/json.ml Outdated Show resolved Hide resolved
src/sail_json_backend/json.ml Outdated Show resolved Hide resolved
@snapdgn snapdgn marked this pull request as ready for review May 3, 2024 02:25
src/sail_json_backend/json.ml Outdated Show resolved Hide resolved
src/sail_json_backend/json.ml Outdated Show resolved Hide resolved
src/sail_json_backend/json.ml Outdated Show resolved Hide resolved
src/sail_json_backend/json.ml Show resolved Hide resolved
src/sail_json_backend/json.ml Outdated Show resolved Hide resolved
src/sail_json_backend/json.ml Outdated Show resolved Hide resolved
src/sail_json_backend/json.ml Outdated Show resolved Hide resolved
src/sail_json_backend/json.ml Outdated Show resolved Hide resolved
Copy link
Owner

@ThinkOpenly ThinkOpenly left a comment

Choose a reason for hiding this comment

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

3 minor comments, inline.

[when is new to me, but I understand it adds an additional condition for the entire case to be satisfied. Neat!]

src/sail_json_backend/json.ml Outdated Show resolved Hide resolved
src/sail_json_backend/json.ml Outdated Show resolved Hide resolved
src/sail_json_backend/json.ml Outdated Show resolved Hide resolved
Copy link
Owner

@ThinkOpenly ThinkOpenly left a comment

Choose a reason for hiding this comment

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

2 very minor comments, and a small refactor request. (Then, I think we're good.)

src/sail_json_backend/json.ml Outdated Show resolved Hide resolved
src/sail_json_backend/json.ml Outdated Show resolved Hide resolved
src/sail_json_backend/json.ml Outdated Show resolved Hide resolved
@snapdgn
Copy link
Author

snapdgn commented May 22, 2024

Sorry about the recent commits. I was working on this issue and accidentally committed here. I've since reverted the changes.

@snapdgn
Copy link
Author

snapdgn commented May 23, 2024

Let me know if you have any further feedback/suggestion. If this looks good, should i go ahead and squash the commits, with a proper descriptive message?

@ThinkOpenly
Copy link
Owner

Let me know if you have any further feedback/suggestion. If this looks good, should i go ahead and squash the commits, with a proper descriptive message?

No further comments. I was going to squash and merge, but you would be able to create your preferred commit if you squash. Thanks!

This commit enhances the JSON Backend to properly handle instructions with optional operands.
Previously, these operands were not handled correctly.

The implementation introduces two new fields:
`optional`, which indicates whether an operand is optional (boolean),
and `default`, which holds the default value if no value is specified.

for 'vle16.v':

Before:
```
{
  "operands": [
    ...
    { "name": "vm", "type": "bits(1)" }
    ...
  ]
}
```
After:
```
{
  "operands": [
    ...
    { "name": "vm", "type": "bits(1)", "optional": true, "default": "v0.t" }
    ...
  ]
}
```
Fixes: ThinkOpenly#2
@snapdgn
Copy link
Author

snapdgn commented May 23, 2024

Squashed the commits.Let me know if you'd like any modifications to the commit message. Otherwise, I think we're ready to proceed with the merge.

@ThinkOpenly ThinkOpenly merged commit 94e3038 into ThinkOpenly:json May 23, 2024
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