[RISCV] Implement encoding for CI-type instructions in terms of its Shift. NFCI#1269
Draft
Jonathon Penix (jonathonpenix) wants to merge 2 commits into
Draft
[RISCV] Implement encoding for CI-type instructions in terms of its Shift. NFCI#1269Jonathon Penix (jonathonpenix) wants to merge 2 commits into
Shift. NFCI#1269Jonathon Penix (jonathonpenix) wants to merge 2 commits into
Conversation
Jonathon Penix (jonathonpenix)
force-pushed
the
pr/ci_type_imm_shift
branch
from
653b725 to
7770fcc
Compare
Jonathon Penix (jonathonpenix)
force-pushed
the
pr/ci_type_imm_shift
branch
from
7770fcc to
0587ad2
Compare
This code should be dead as `R_RISCV_RVC_LUI` is an internal relocation to help with relaxation and the only place we emit this relocation already checks that the immediate is non-zero. Cleaning this up is also helpful as a follow up patch will introduce a new internal relocation for `c.li` (for handling GOT load relaxations). The existing code is incorrect for that case, and cleaning this up will let us handle these two consistently (and more consistently with other instruction types). Signed-off-by: Jonathon Penix <jpenix@qti.qualcomm.com>
…Shift`. NFCI A follow-up patch will add a new internal relocation to handle `c.li`. Currently, `encodeCI` looks for the immediate in the higher bits of `Result` (bits 17 and 16:12) which makes sense for `c.lui` handling but not really for `c.li` (where we want to look at bits 5 and 4:0). I don't think this would work as-is for some of the other CI-type instructions if they ever need to be handled so it isn't entirely generic. But, this should be sufficient for supporting both `c.li` and `c.lui`. I also think this is marginally simpler/more straightforward than creating a separate `EncTy_*` or checking the instruction type, etc. so I went this route for now. Note `isValidRVCLUIType` is now in a (more) broken state as I didn't update the call to `encodeCI`. As far as I can tell, this code is already dead/broken (it ultimately calls `extractRVCImmediate` which itself has a typo (`exractBits`) so has never been compile-able as far as I can tell. Not sure if we want to fix this up or just remove it, so I'm leaving it be for the purposes of this patch. Signed-off-by: Jonathon Penix <jpenix@qti.qualcomm.com>
Jonathon Penix (jonathonpenix)
force-pushed
the
pr/ci_type_imm_shift
branch
from
0587ad2 to
8dcc4f0
Compare
Jonathon Penix (jonathonpenix)
requested a review
from Shankar Easwaran (quic-seaswara)
as a code owner
Sudharsan Veeravalli (svs-quic)
requested review from
Sam Elliott (lenary)
and removed request for
Sudharsan Veeravalli (svs-quic)
Sam Elliott (lenary)
approved these changes
Jun 22, 2026
Sam Elliott (lenary)
left a comment
Sam Elliott (lenary)
left a comment
Member
There was a problem hiding this comment.
LGTM.
Instruction types are a bit annoying in the C extension, potentially less relevant, but this seems the best approach.
Shankar Easwaran (quic-seaswara)
approved these changes
Jun 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A follow-up patch will add a new internal relocation to handle
c.li.Currently,
encodeCIlooks for the immediate in the higher bits ofResult(bits 17 and 16:12) which makes sense for
c.luihandling but not really forc.li(where we want to look at bits 5 and 4:0).I don't think this would work as-is for some of the other CI-type instructions
if they ever need to be handled so it isn't entirely generic. But, this should
be sufficient for supporting both
c.liandc.lui. I also think this ismarginally simpler/more straightforward than creating a separate
EncTy_*orchecking the instruction type, etc. so I went this route for now.
Note
isValidRVCLUITypeis now in a (more) broken state as I didn't update thecall to
encodeCI. As far as I can tell, this code is already dead/broken (itultimately calls
extractRVCImmediatewhich itself has a typo (exractBits)so has never been compile-able as far as I can tell. Not sure if we want to fix
this up or just remove it, so I'm leaving it be for the purposes of this patch.
======
Stacked on #1268