Skip to content

Fix vector shift opcodes on x86 - #8101

Merged
0xdaryl merged 1 commit into
eclipse-omr:masterfrom
gita-omr:fix_intel_vector_shifts
Feb 2, 2026
Merged

Fix vector shift opcodes on x86#8101
0xdaryl merged 1 commit into
eclipse-omr:masterfrom
gita-omr:fix_intel_vector_shifts

Conversation

@gita-omr

@gita-omr gita-omr commented Jan 23, 2026

Copy link
Copy Markdown
Contributor
  • Correct the entries for BinaryLogicalShiftRight and BinaryArithmeticShiftRight
    opcodes in theVectorBinaryArithmeticOpCodesForMem table to use SRL and
    SRA vector instructions
  • Format the tables for human readability

@0xdaryl

0xdaryl commented Jan 23, 2026

Copy link
Copy Markdown
Contributor

Can your commit message be more specific on what was "fixed" please? Is this simply a source code formatting change?

Also, please say "x86" rather than "Intel" in your wording as this isn't an Intel-specific fix.

@dsouzai dsouzai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You'll need something like this

// clang-format off
...
// clang-format on

to prevent the linter failures. See #7883

@gita-omr

Copy link
Copy Markdown
Contributor Author

Can your commit message be more specific on what was "fixed" please? Is this simply a source code formatting change?

Also, please say "x86" rather than "Intel" in your wording as this isn't an Intel-specific fix.

It's not just a formatting change. It fixes eclipse-openj9/openj9#23195

The fix is hard to see because the table was unreadable. I will update the message.

@gita-omr
gita-omr force-pushed the fix_intel_vector_shifts branch from daeba24 to d69af46 Compare January 23, 2026 15:01
@gita-omr gita-omr changed the title Fix vector shift opcodes on Intel Fix vector shift opcodes on x86 Jan 23, 2026
@gita-omr
gita-omr force-pushed the fix_intel_vector_shifts branch from d69af46 to d3ef0b9 Compare January 23, 2026 15:03
@gita-omr

Copy link
Copy Markdown
Contributor Author

Addressed the comments.

@0xdaryl

0xdaryl commented Jan 27, 2026

Copy link
Copy Markdown
Contributor

The comment is difficult to parse. I suggest something like:

- Correct the entries for BinaryLogicalShiftRight and BinaryArithmeticShiftRight 
opcodes in  the`VectorBinaryArithmeticOpCodesForMem` table to use SRL and 
SRA vector instructions
- Format the tables for human readability

In the future, it would be better to separate the functional change (correcting the opcodes) from the code formatting change into separate commits. It was extremely tedious to isolate the subtle functional changes made in this PR using either GitHub or offline diff tools.

@gita-omr

Copy link
Copy Markdown
Contributor Author

In the future, it would be better to separate the functional change (correcting the opcodes) from the code formatting change into separate commits. It was extremely tedious to isolate the subtle functional changes made in this PR using either GitHub or offline diff tools.

It was also extremely tedious to find the bug. Only a single entry in the table caused the test failure. To identify all the typos (six so far), the table had to be reformatted. As a result, formatting and fixes became interleaved. Even if the formatting had been done separately, the diff would still have been difficult to decipher, since the original input was unreadable to begin with. On the other hand, there are offline tools that handle this case quite well.

- Correct the entries for BinaryLogicalShiftRight and BinaryArithmeticShiftRight
  opcodes in  the`VectorBinaryArithmeticOpCodesForMem` table to use SRL and
  SRA vector instructions
- Format the tables for human readability
@gita-omr
gita-omr force-pushed the fix_intel_vector_shifts branch from d3ef0b9 to a74d0c3 Compare January 30, 2026 16:56
@gita-omr

gita-omr commented Feb 2, 2026

Copy link
Copy Markdown
Contributor Author

Addressed comments.

@0xdaryl
0xdaryl merged commit 77b735f into eclipse-omr:master Feb 2, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants