Skip to content

DynComp: treat the remaining vector shifts as shifts - #88

Merged
markro49 merged 4 commits into
codespecs:masterfrom
mernst:dyncomp-vector-shifts
Oct 17, 2026
Merged

DynComp: treat the remaining vector shifts as shifts#88
markro49 merged 4 commits into
codespecs:masterfrom
mernst:dyncomp-vector-shifts

Conversation

@mernst

@mernst mernst commented Oct 10, 2026

Copy link
Copy Markdown
Member

Iop_QSal*, Iop_QShl* (64-bit variants), Iop_Rol64x2, and Iop_SarV128 were in DynComp's list of unhandled binary operators, so Kvasir panicked on them. They are shifts, like their 128-bit counterparts that DynComp already handles, so this PR handles them the same way: the result gets the tag of the shifted value, and the shift amount does not interact with it.

Split out of #85.

@mernst
DynComp: treat the remaining vector shifts as shifts
c7161e2 
Iop_QSal*, Iop_QShl* (64-bit variants), Iop_Rol64x2, and Iop_SarV128
are shifts, like their 128-bit counterparts.  The result gets the tag of
the shifted value, and the shift amount does not interact with it.
@coderabbitai

coderabbitai Bot commented Oct 10, 2026 •
edited
Loading

Copy link
Copy Markdown

Review in Change Stack →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration
  • Configuration used: Repository: codespecs/fjalar/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: a0cc5017-f750-4fdd-b7c5-edb603629901
📥 Commits

Reviewing files that changed from the base of the PR and between c2a62d3 and ec58927.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: codespecs/fjalar/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 60f12ca3-c8ac-402d-a0a2-46e42c515854
📥 Commits

Reviewing files that changed from the base of the PR and between ea8fa94 and c2a62d3.

📒 Files selected for processing (1)
  • valgrind/fjalar/kvasir/dyncomp_translate.c

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

expr2tags_Binop_DC now classifies additional ARM, ARM64, and S390 shift operations to return the first operand’s tag. The unsupported-opcode list removes several operations now handled in the binary-operation switch and separates binary and unary entries.

Priority: ➖ Normal

Change: Bug fix

Merge Risk: ⚪ Minimal · up to c2a62

This change stops DynComp from panicking on the remaining vector shift operations by giving the result the shifted value's tag. No concrete merge-blocking risk was found.

### Architecture Summary

Architecture risk: 🔵 Low · up to c2a62

The change affects 1 system.

Changed systems: valgrind

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — valgrind (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in valgrind/fjalar/kvasir/dyncomp_translate.c: The vector shift heading now includes both scalar and vector shift forms, replacing the scalar-only heading.
  • observed — Modified behavior in valgrind/fjalar/kvasir/dyncomp_translate.c: Adds ARM saturating shift opcodes to the cases that return the first operand’s tag, so their shift amounts are not merged into the result tag.
  • observed — Modified behavior in valgrind/fjalar/kvasir/dyncomp_translate.c: The 128-bit SIMD shift heading now covers scalar and vector shifts; the existing rotate cases remain under it.
  • observed — Modified behavior in valgrind/fjalar/kvasir/dyncomp_translate.c: Adds the S390 Iop_SarV128 case to the first-operand-tag path and comments that BCD add/sub operations are arithmetic that may warrant merging operand tags.
Pre-merge checks | Passed 2 | Failed 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

mernst added 2 commits October 10, 2026 12:38
@mernst
DynComp: handle the other vector shifts, and regroup shift cases
644ef63 
Treat Iop_QShlNsat*, Iop_Sh*, Iop_Rsh*, Iop_QandSQ*sh*, Iop_QandUQ*sh*,
and Iop_QandQ*Nnarrow* as shifts, rather than panicking on them.

Move the 64-bit saturating shifts into the 64-bit shift group, Iop_Rol64x2
next to the other rotates, and Iop_SarV128 next to Iop_ShlV128 and
Iop_ShrV128.  Correct the group headings and the comment about
unimplemented opcodes.
@mernst
DynComp: clarify comments about vector shifts and unimplemented ops
c2a62d3 
Document that Qand* ops tag their QC-flag half with the data tag,
that per-lane VECTOR x VECTOR shift amounts are treated like scalar
shift amounts, and that the BCD ops are not shifts.  Say which
function each unimplemented op belongs in, and drop the VDup ops,
which are not IROps in libvex_ir.h.
@mernst

mernst commented Oct 11, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 11, 2026 •
edited
Loading

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@mernst

mernst commented Oct 11, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 11, 2026 •
edited
Loading

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@mernst

mernst commented Oct 11, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 11, 2026 •
edited
Loading

Copy link
Copy Markdown
⚠️ Action not completed

Already reviewed the last commit. Use @coderabbitai full review to rerun a review of the entire changeset.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@mernst

mernst commented Oct 11, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 11, 2026 •
edited
Loading

Copy link
Copy Markdown
⚠️ Action not completed

Already reviewed the last commit. Use @coderabbitai full review to rerun a review of the entire changeset.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@mernst

mernst commented Oct 11, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 11, 2026 •
edited
Loading

Copy link
Copy Markdown
⚠️ Action not completed

Already reviewed the last commit. Use @coderabbitai full review to rerun a review of the entire changeset.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@mernst

mernst commented Oct 11, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 11, 2026 •
edited
Loading

Copy link
Copy Markdown
⚠️ Action not completed

Already reviewed the last commit. Use @coderabbitai full review to rerun a review of the entire changeset.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@mernst
mernst requested a review from markro49 October 11, 2026 19:34
@mernst

mernst commented Oct 11, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 11, 2026 •
edited
Loading

Copy link
Copy Markdown
⚠️ Action not completed

Already reviewed the last commit. Use @coderabbitai full review to rerun a review of the entire changeset.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

mernst added a commit to mernst/fjalar that referenced this pull request Oct 17, 2026
@mernst
Plans: account for open DynComp PRs codespecs#85-codespecs#88 and DWA…
c326dfd 
…RF PRs codespecs#89-codespecs#95

Both upgrades should wait for them: codespecs#89, codespecs#90, codespecs#91, and codespecs#95 change dwarf.c
(and codespecs#90/codespecs#95 change mc_main.c and mc_include.h), and codespecs#85-codespecs#88 change
dyncomp_translate.c.  codespecs#85's --dyncomp-unhandled-ops=warn and codespecs#87's
explicit unary handling also change how to find unhandled IROps.
@markro49
markro49 merged commit 125330a into codespecs:master Oct 17, 2026
6 checks passed
@markro49
markro49 deleted the dyncomp-vector-shifts branch October 17, 2026 18:21
@mernst mernst mentioned this pull request Oct 18, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markro49 markro49 markro49 approved these changes

Assignees

@markro49 markro49

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mernst @markro49