Skip to content

Conversation

@james-ball-qualcomm
Copy link
Contributor

This is a replacement for PR RISC-V-Certification-Steering-Committee#175 which had too many unrelated commits and file changes for some reason.

##Credits / Contributions

Ammarah wrote the normative rules for the B extension.
@ayeshaanwaar05 wrote the normative rules for the instructions.
@EmanNasar001 authored the YAML file

- name: slli-uw_ind
tags: ["norm:slli-uw_ind"]

# === Zbb: Basic Bit-Manipulation ===
Copy link
Contributor

Choose a reason for hiding this comment

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

Many of these extension level normative rules seem like they are general overview and not normative text. More specific versions of them are repeated in the instructions section below, so I'd drop the overview rules/tags. I commented on a few of them, but there are several others.

Choose a reason for hiding this comment

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

Thanks for the feedback. Should I make a follow-up PR to remove those tags from the extension level sections and keep them only at the instruction level?

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be great. The easiest way to do so right now is for you to create a branch in your CSC fork based on the B-ext-merge branch (https://github.com/RISC-V-Certification-Steering-Committee/riscv-isa-manual/tree/B-ext-merge). Then you can create a PR on the CSC fork targeting the B-ext-merge (instead of main) and it will update this PR automatically when it is merged. We'll figure out a better system soon, but this will work until that's sorted out.

Copy link

@Ammarahwakeel Ammarahwakeel left a comment

Choose a reason for hiding this comment

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

I may be missing something, but I did run the Makefile and resolved similar errors. Some of those were related to tags being in the .adoc file instead of the YAML file, and vice versa. At the time I opened the PR, there were no errors and the draft PDF built correctly, so I’m wondering how this orc-b_behave error slipped through.

@james-ball-qualcomm
Copy link
Contributor Author

I may be missing something, but I did run the Makefile and resolved similar errors. Some of those were related to tags being in the .adoc file instead of the YAML file, and vice versa. At the time I opened the PR, there were no errors and the draft PDF built correctly, so I’m wondering how this orc-b_behave error slipped through.

As I just discussed in another comment, we've apparently fallen into an unexpected aspect of Git related to forks. Most of my development in Git has been in projects where I just made changes in a branch and didn't need to use a fork like the ISA manual requires of you (unless you have higher privileges than I do). So, I was caught off guard when PR #2518 was merged a few days ago and it followed typical Git behavior of squashing all the hundreds of commits in that PR down to one in the upstream. Since Git relies on commits and not file content differences like other source control solutions (e.g., Perforce, SCCS, etc.) now Git thinks that our CSC fork's main and all the forks of it (like yours) and all the branches and PRs off of main (like we have for each chapter in the ISA manual) have hundreds of commits ahead of the upstream and so doing a PR from these is a complete mess.

Two work around this, @jordancarlin helped me yesterday to create a new-main branch and the B-ext-merge branch. We had to use the git cherry-pick facility to move your commits from its original branch off main into B-ext-merge. This is a somewhat error prone process and we did resolve merge conflicts the best we could. Seems like we didn't get everything which is concerning but not completely unexpected when doing a cherry-pick so then I had to fix up some remaining issues like you've noticed.

Other like @rpsene and @mmhus ([email protected]) should see this too.

I'll be working with Moiz today (new CSC TPM working for RVI) to figure out our way out of this mess. It's going to be the case for all the many branches and PRs we've created in our CSC fork for each of the chapters. So we'll need a more robust and scalable solution then this cherry-pick solution.

@wmat
Copy link
Collaborator

wmat commented Jan 9, 2026 via email

@Ammarahwakeel
Copy link

I may be missing something, but I did run the Makefile and resolved similar errors. Some of those were related to tags being in the .adoc file instead of the YAML file, and vice versa. At the time I opened the PR, there were no errors and the draft PDF built correctly, so I’m wondering how this orc-b_behave error slipped through.

As I just discussed in another comment, we've apparently fallen into an unexpected aspect of Git related to forks. Most of my development in Git has been in projects where I just made changes in a branch and didn't need to use a fork like the ISA manual requires of you (unless you have higher privileges than I do). So, I was caught off guard when PR #2518 was merged a few days ago and it followed typical Git behavior of squashing all the hundreds of commits in that PR down to one in the upstream. Since Git relies on commits and not file content differences like other source control solutions (e.g., Perforce, SCCS, etc.) now Git thinks that our CSC fork's main and all the forks of it (like yours) and all the branches and PRs off of main (like we have for each chapter in the ISA manual) have hundreds of commits ahead of the upstream and so doing a PR from these is a complete mess.

Two work around this, @jordancarlin helped me yesterday to create a new-main branch and the B-ext-merge branch. We had to use the git cherry-pick facility to move your commits from its original branch off main into B-ext-merge. This is a somewhat error prone process and we did resolve merge conflicts the best we could. Seems like we didn't get everything which is concerning but not completely unexpected when doing a cherry-pick so then I had to fix up some remaining issues like you've noticed.

Other like @rpsene and @mmhus ([email protected]) should see this too.

I'll be working with Moiz today (new CSC TPM working for RVI) to figure out our way out of this mess. It's going to be the case for all the many branches and PRs we've created in our CSC fork for each of the chapters. So we'll need a more robust and scalable solution then this cherry-pick solution.

Oh, that makes sense now. Thank you for the detailed explanation. I see why my PR showed 200+ commits instead of just my own four, and I understand how the fork and squash merge behavior caused this complexity. I appreciate all the effort in managing the cherry-picks and branch restructuring to merge a single PR.

@wmat
Copy link
Collaborator

wmat commented Jan 9, 2026 via email

@james-ball-qualcomm
Copy link
Contributor Author

@Ammarahwakeel, thanks for the understanding. I'm working with @jrgrimm11, @jordancarlin, and @mmhus to straighten this out.

@wmat, are you saying the ISA manual doesn't have an intentional policy of requiring forks instead of branches? Branches would be easier to deal with so I'm a fan of that. There must be some other tradeoffs involved but that's not my forte.

@wmat
Copy link
Collaborator

wmat commented Jan 11, 2026 via email

@UmerShahidengr
Copy link
Contributor

@james-ball-qualcomm I am a bit confused here, why is this PR made directly into riscv/riscv-isa-manual instead of csc fork for CSC review. Have the CSC reviewers reviewed this chapter?

@james-ball-qualcomm
Copy link
Contributor Author

You should talk to Jamie and Moiz.

@mmhus mmhus added the NormRules This PR adds new normative rules or modifies existing ones or adds infrastructure label Jan 28, 2026
@UmerShahidengr
Copy link
Contributor

I have reviewed and updated the B-extension normative rules, new PR has been created here: #2628 . This one can be closed now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NormRules This PR adds new normative rules or modifies existing ones or adds infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants