Bitmanip chapter integration.

[wmat]

Integration of the bitmanip specification into riscv-isa-manual.

[wmat]

Hi folks, just wondering what the next steps are here? I'd like to see this chapter merged soon.

[aswaterman]

Do you know who's supposed to be reviewing it?

[wmat]

@ptomsich would you be the primary person to review here, or if not, who would you recommend?

[ptomsich]

@ptomsich would you be the primary person to review here, or if not, who would you recommend?

I am planning to act as a reviewer, but can only start once I am back in my home time-zone end of next week.

Please add me as a reviewer.

[wmat]

Excellent, thanks @ptomsich

[wmat]

@ptomsich just checking in to see if review of this PR is progressing? I'd like to get the bitmanip chapter accepted into the main document asap.

Thanks
Bill

[wmat]

@ptomsich please let me know if there's anything you need from me to move forward your review of this integration? I can provide the PDF of unpriv with bitmanip integrated if that will help.

[wmat]

@ptomsich just wanted to check in to see if you still intended to review the bitmanip chapter?

[wmat]

Note that this PR has been outstanding for 5 months now. With respect to moving forward I am planning to merge this PR on Thursday, Feb. 1st. Any issues that may be found post merge can be handled in the main branch. @ptomsich @aswaterman @jjscheel

[topperc]

This seems to be the only instruction that uses the convention of putting "(RV64)" here if it's not supported on RV32. Should all the other W instructions do the same or should this be dropped for this instruction?

[ptomsich]

Note that this PR has been outstanding for 5 months now. With respect to moving forward I am planning to merge this PR on Thursday, Feb. 1st. Any issues that may be found post merge can be handled in the main branch. @ptomsich @aswaterman @jjscheel

Sorry, I am short on bandwidth.
Please note that there are a number of outstanding pull-requests for the original repository (https://github.com/riscv/riscv-bitmanip/pulls) and this specification hasn't had an editor in a long time (i.e., I only helped out with the conversion to the new-style specification, but never was the editor).

[gfavor]

Just adding on to Philipp's post: There are known issues/errors in the bitmanip spec - as reflected in the outstanding PRs in the original repo. These HAVE to be addressed. I don't know if they can simply be brought over and merged into the adoc spec, or if straightforward editing needs to be done that Bill can do, or if someone else needs to do more technical editing to create suitable changes for Bill to incorporate.

So maybe first step is for Bill to review the PRs from the original repo and let the rest of know where we stand as far as what is needed to incorporate that stuff into the bitmanip spec.

[wmat]

I'm happy to follow up on the 7 outstanding PRs in the bitmanip repository and pull in whatever I can. Note that they do go back as far as March 2021.

[gfavor]

Great. Let us know whether all 7 can be pulled in and/or whether there is further work that is still needed (that you can't do on your own). Greg

…

On Tue, Jan 30, 2024 at 6:05 AM Bill Traynor ***@***.***> wrote: I'm happy to follow up on the 7 outstanding PRs in the bitmanip repository and pull in whatever I can. Note that they do go back as far as March 2021. — Reply to this email directly, view it on GitHub <#1087 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALLX6GVFOZIJDQSNZ2TAVK3YRD43TAVCNFSM6AAAAAA3IVRLOKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJWHEZTINZWHE> . You are receiving this because you commented.Message ID: ***@***.***>

[wmat]

Great. Let us know whether all 7 can be pulled in and/or whether there is further work that is still needed (that you can't do on your own). Greg
…
On Tue, Jan 30, 2024 at 6:05 AM Bill Traynor @.> wrote: I'm happy to follow up on the 7 outstanding PRs in the bitmanip repository and pull in whatever I can. Note that they do go back as far as March 2021. — Reply to this email directly, view it on GitHub <#1087 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALLX6GVFOZIJDQSNZ2TAVK3YRD43TAVCNFSM6AAAAAA3IVRLOKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJWHEZTINZWHE . You are receiving this because you commented.Message ID: @.>

Hi @gfavor , @ptomsich , @jjscheel ,

Here's a review of the 7 outstanding PRs. I can't comment whether they should be merged or not, I'll need @ptomsich or someone to make that decision. However, if each should have been merged, please comment here as I'll need to make sure I pull them into my PR. Here's the simple review results:

• First 3 PRs require someone's review to merge or abandon.
• Fourth PR can be merged as is.
• Last 3 PRs can be closed and not merged as they no longer apply.

Here's a bit more explanation:

PR#182 Replace PTRLOG with explicit 3 #182

"The shift immediate should be 3, even for rv32, since we want log(bits-per-byte), not log(bytes-per-word). PTRLOG implies log(bytes-per-word), so replace it with an explicit 3 as is used elsewhere in the example when log(bits-per-byte) is needed."

Seems a very simple change, just need someone to give it a thumbs up or not.

PR#181 show opcode as 7-bits like all other insn for unzip and zip

"[ 00100 ][ 11 ] should be written as [ 0010011 ] (0x13) or OP-IMM to be consistent with all other instructions.

Also, in the Operations section, should one of the foreach loops between unzip and zip be different like, for example,

foreach(i from 0 to xlen/2-1) {
x(rd)[i] = X(rs1)[2i]
x(rd)[2i+1] = X(rs1)[i+xlen/1]
}
"
Another simple change, just need someone to give it a thumbs up or not.

PR#180 insn is I-type, opcode should be OP-IMM[-32] for zext.h

"Instructions with bits [24:20] as immediate or function values are I-type. Instructions with bits [24:20] as registers (like rs2) are R-type. Shouldn't zext.b be an I-type instruction, with opcode OP-IMM or 0x13?"

Another simple change, just need someone to give it a thumbs up or not.

PR#172 Fix desc for signextend

This change is technically correct according to the comments but was not merged as it "has no significant impact on the specification."

PR#130 Remove 16-bit encodings

"The compressed future instructions section caused encoding clashes with the code-size reduction Zce, and caused confusion in the PLCT implementation of the tools, so I've removed them. C.NOT, C.NEG, and C.ZEXT.W are included in Zce already. C.ZEXT.D isn't currently included in Zce as it is considers instructions for RV32/RV64 but does not specifically add instructions for RV128"

This PR received no comments and is no longer relevant as the file it changes no longer exists in the src tree. I believe this PR can simply be closed and not merged.

PR#126 Update BCOMPRESS / BDECOMPRESS -> BMEXT / BMDEP in opcode tables

"Updated the BCOMPRESS / BDECOMPRESS names in the opcode tables to the new BMEXT / BMDEP. Also swapped the BMEXT / BMDEP location in the funct7 / funct3 mapping table. I believe previously those were incorrect / swapped in that table, but double-check that aspect of the PR."

This PR no longer applies and can be closed and not merged.

PR#123

"This commit fixes a number of assembler coding issues and also a missing MACRO definition file".

This PR no longer applies as these files are no longer in the src tree and are specific to tests, not the body of the specification.

Overall, nothing here fundamentally changes the spec and can be easily reverted if need be. My recommendation is to merge the first 4 PRs and abandon the last 3.

Thanks
Bill

[wmat]

Great. Let us know whether all 7 can be pulled in and/or whether there is further work that is still needed (that you can't do on your own). Greg
…
On Tue, Jan 30, 2024 at 6:05 AM Bill Traynor @.> wrote: I'm happy to follow up on the 7 outstanding PRs in the bitmanip repository and pull in whatever I can. Note that they do go back as far as March 2021. — Reply to this email directly, view it on GitHub <#1087 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALLX6GVFOZIJDQSNZ2TAVK3YRD43TAVCNFSM6AAAAAA3IVRLOKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJWHEZTINZWHE . You are receiving this because you commented.Message ID: _@**.**_>

Hi @gfavor , @ptomsich , @jjscheel ,

Here's a review of the 7 outstanding PRs. I can't comment whether they should be merged or not, I'll need @ptomsich or someone to make that decision. However, if each should have been merged, please comment here as I'll need to make sure I pull them into my PR. Here's the simple review results:

• First 3 PRs require someone's review to merge or abandon.
• Fourth PR can be merged as is.
• Last 3 PRs can be closed and not merged as they no longer apply.

Here's a bit more explanation:

PR#182 Replace PTRLOG with explicit 3 #182

"The shift immediate should be 3, even for rv32, since we want log(bits-per-byte), not log(bytes-per-word). PTRLOG implies log(bytes-per-word), so replace it with an explicit 3 as is used elsewhere in the example when log(bits-per-byte) is needed."

Seems a very simple change, just need someone to give it a thumbs up or not.

PR#181 show opcode as 7-bits like all other insn for unzip and zip

"[ 00100 ][ 11 ] should be written as [ 0010011 ] (0x13) or OP-IMM to be consistent with all other instructions.

Also, in the Operations section, should one of the foreach loops between unzip and zip be different like, for example,

foreach(i from 0 to xlen/2-1) { x(rd)[i] = X(rs1)[2_i] x(rd)[2_i+1] = X(rs1)[i+xlen/1] } " Another simple change, just need someone to give it a thumbs up or not.

PR#180 insn is I-type, opcode should be OP-IMM[-32] for zext.h

"Instructions with bits [24:20] as immediate or function values are I-type. Instructions with bits [24:20] as registers (like rs2) are R-type. Shouldn't zext.b be an I-type instruction, with opcode OP-IMM or 0x13?"

Another simple change, just need someone to give it a thumbs up or not.

PR#172 Fix desc for signextend

This change is technically correct according to the comments but was not merged as it "has no significant impact on the specification."

PR#130 Remove 16-bit encodings

"The compressed future instructions section caused encoding clashes with the code-size reduction Zce, and caused confusion in the PLCT implementation of the tools, so I've removed them. C.NOT, C.NEG, and C.ZEXT.W are included in Zce already. C.ZEXT.D isn't currently included in Zce as it is considers instructions for RV32/RV64 but does not specifically add instructions for RV128"

This PR received no comments and is no longer relevant as the file it changes no longer exists in the src tree. I believe this PR can simply be closed and not merged.

PR#126 Update BCOMPRESS / BDECOMPRESS -> BMEXT / BMDEP in opcode tables

"Updated the BCOMPRESS / BDECOMPRESS names in the opcode tables to the new BMEXT / BMDEP. Also swapped the BMEXT / BMDEP location in the funct7 / funct3 mapping table. I believe previously those were incorrect / swapped in that table, but double-check that aspect of the PR."

This PR no longer applies and can be closed and not merged.

PR#123

"This commit fixes a number of assembler coding issues and also a missing MACRO definition file".

This PR no longer applies as these files are no longer in the src tree and are specific to tests, not the body of the specification.

Overall, nothing here fundamentally changes the spec and can be easily reverted if need be. My recommendation is to merge the first 4 PRs and abandon the last 3.

Thanks Bill

@gfavor I did not receive any feedback on these 7 PRs from the original Bitmanip repository. I'd like to merge Bitmanip into Unpriv today and archive the original Bitmanip repository. So should I apply the 3 PRs I noted in my review to the Bitmanip PR against Unpriv, or should I abandon them and perhaps note in the original repo PR that if they still apply, the author should re-make the change against riscv-isa-manual?

[gfavor]

@ptomsich I think it is important/valuable to have Philipp weigh in on #1087 (comment) and whether he agrees with Bill's suggested dispositions for each of the 7 PRs. I would go with what he suggests.

[aswaterman]

At @wmat's request, I've reviewed those PRs. The following should be merged:

• Fix desc for signextend riscv-bitmanip#172
• show opcode as 7-bits like all other insn for unzip and zip riscv-bitmanip#181
• strlen: Replace PTRLOG with explicit 3 riscv-bitmanip#182

The following target an old branch and are no longer relevant, so should be closed:

• Fix test for recent 0.93 changes riscv-bitmanip#123
• Update BCOMPRESS / BDECOMPRESS -> BMEXT / BMDEP in opcode tables riscv-bitmanip#126
• Remove 16-bit encodings riscv-bitmanip#130

The following is a semantic change, which is inappropriate for a ratified spec, and so should be closed:

• insn is I-type, opcode should be OP-IMM[-32] for zext.h riscv-bitmanip#180

[wmat]

All updates have been made as per @aswaterman approvals above.

[ptomsich]

The zext.w is a 32-bit zero-extension; it is only a pseudo, but likely one of the most common usages of the add.wu instruction.

[wmat]

Does that mean I should revert this change?

[ptomsich]

I am pointing out that we have a 32bit zero-extension as one of our (pseudo-)instructions.

Strictly speaking, zext.w is not an instruction but a special-case of add.uw.
So strictly speaking, not listing 32 bit is correct — however, as a reader I would expect the 32 bit included (i.e., the change should be reverted).

It's a judgement call.

[ptomsich]

It's a judgement call that a native speaker should make.

[wmat]

Are you OK with accepting the approved change as is and allowing for any changes against the Chapter after it's merged into Unpriv?

[topperc]

If I'm reading the document right, we're in a Zbb section of the document describing the sext/zext instructions in Zbb. zext.w is a pseudoinstruction from Zba so wouldn't belong in this section I think.

[wmat]

Thanks @topperc , I am going to merge Bitmanip into Unpriv today, would you mind raising this as an issue against riscv-isa-manual after that happens?

[topperc]

I'm not sure what issue you want me to raise. The zext.w pseudoinstruction is not listed in this section and shouldn't be listed in this section since its a Zbb section. Therefore only mentioning 8 and 16 bit here is correct given this structure.

[wmat]

Oh my bad, I see your point now.

[aswaterman]

I agree with @topperc that, taken in the context of this section within the Zbb spec, the change in the PR was correct. We don't need to take any further action.