Skip to content

Stop analysis at illegal instructions - #13

Merged
jserv merged 1 commit into
sysprog21:mainfrom
thc1006:rvopt-illegal-block-end
Aug 3, 2026
Merged

Stop analysis at illegal instructions#13
jserv merged 1 commit into
sysprog21:mainfrom
thc1006:rvopt-illegal-block-end

Conversation

@thc1006

@thc1006 thc1006 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #12: decoding unsupported JALR and SYSTEM encodings as K_ILL exposed a pre-existing CFG gap where illegal instructions did not terminate analysis, allowing constant propagation to cross unreachable words and resolve a later JALR.

Unsupported JALR and SYSTEM encodings are decoded as K_ILL. K_ILL does not end a basic block, so constant propagation can cross an unreachable illegal word and resolve a later JALR.

Make K_ILL end the block and stop CFG traversal. Add jump-over cases for a reserved JALR and a CSR word. Also stop the encoding test when PACK_WORDS fails.

make check-all passes.


Summary by cubic

Stop CFG traversal at illegal instructions to prevent constant propagation from crossing unreachable words and wrongly resolving later JALR. Treat K_ILL as a basic-block end and a terminator; add jump-over tests and fail early on encoding errors.

  • Bug Fixes
    • is_block_end() now treats K_ILL as a block terminator.
    • successors() stops traversal at K_ILL.
    • Added jump-over tests for reserved JALR and CSR so unreachable illegal words don’t affect reachable paths.
    • verify-mux now fails if PACK_WORDS cannot encode input.

Written for commit 15ca056. Summary will update on new commits.

Review in cubic

Unsupported JALR and SYSTEM encodings are decoded as K_ILL. K_ILL
does not end a basic block, so constant propagation can cross an
unreachable illegal word and resolve a later JALR.

Make K_ILL end the block and stop CFG traversal. Add jump-over cases
for a reserved JALR and a CSR word. Also stop the encoding test when
PACK_WORDS fails.

make check-all passes.

Signed-off-by: thc1006 <hctsai1006@cs.nctu.edu.tw>
@thc1006
thc1006 force-pushed the rvopt-illegal-block-end branch from 9f281a4 to 15ca056 Compare August 3, 2026 06:17

@jserv jserv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed the control-flow reasoning and the hand-assembled encodings. The change is correct, is a strict improvement (it removes a spurious rejection of valid jump-over-illegal programs), and the two new tests fail without it. One adjacent observation, not related to this PR: the dump command path runs analyze_syscalls/mark_reachable without the resolve_jalr that the emit path runs first, so its ; promotions: diagnostic output can diverge from emit for static-target JALR programs (diagnostic only, no effect on a compiled image).

Comment thread rvopt.c
const bool link = rd == 1 && (nd->kind == K_JAL ||
(nd->kind == K_JALR && nd->target != NONE));
const bool terminates = (nd->kind == K_JAL && !link) ||
const bool terminates = nd->kind == K_ILL || (nd->kind == K_JAL && !link) ||

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This fixes the illegal-word instance of a more general issue rather than the root cause. The underlying trigger is that resolve_jalr marks the target of any resolved JALR as a leader without checking that the JALR is reachable, and analyze_syscalls then clears cprop at that leader. So an unreachable ret with a constant base can corrupt a reachable node's cprop by leaking a constant across any intervening straight-line instruction, not only an illegal one: replace the illegal word in the new test with a plain NOP (00000013) in the same layout and rvopt still rejects the reachable exit path, with and without this change. This PR is a good, targeted fix for the illegal-word case; a follow-up could gate JALR-target leader marking on reachability (iterate resolve_jalr and mark_reachable to a fixpoint) to close the general case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Confirmed. Replacing the illegal word with either 00000013 (nop) or 00000033 (add x0, x0, x0) reproduces the same rejection on both main and this branch, confirming that leader marking from unreachable JALRs is the general issue.

I'll keep this PR scoped to treating K_ILL as a control-flow barrier and address reachability-gated JALR target leader marking in a separate follow-up.

Comment thread Makefile
Comment on lines +77 to +78
jump-over-reserved-JALR:0100006f,01400093,00009067,00008067,05d00893,00000073 \
jump-over-CSR:0100006f,01400093,00001073,00008067,05d00893,00000073

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Good regression coverage: both cases reject without the rvopt.c change (unsupported op at pc 20) and accept with it, so they pin the behavior.

@thc1006

thc1006 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Thank you prof for the adjacent observation. I confirmed that the dump path skips resolve_jalr, while emit_mux runs it before analyze_syscalls and mark_reachable, so the promotion diagnostics can diverge for static-target JALR programs.

I will keep this PR scoped to the illegal-instruction control-flow barrier and cover the dump/emit analysis mismatch in the follow-up, with a regression test for the diagnostic path.


UPDATE(UTC+8 PM 4:16):
Hmm... quite difficult I think will need more time


UPDATE(UTC+8 PM 7:39):
I have completed the broader CFG follow-up locally. The root cause extends beyond JALR: the initial leader construction marks the targets of direct jumps and branches without considering whether the source instruction is reachable. I confirmed the same spurious rejection with unreachable jal and beq instructions targeting otherwise reachable code.

The broader fix builds on the K_ILL barrier introduced here, but this PR remains self-contained and unchanged in scope. I will open the follow-up after this PR lands. I will keep the dump/emit diagnostic-parity issue in a separate third PR.

@jserv
jserv merged commit 15ca056 into sysprog21:main Aug 3, 2026
6 checks passed
@thc1006
thc1006 deleted the rvopt-illegal-block-end branch August 3, 2026 13:04
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