Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

i#6238: Add categorization of x86 instruction mix and subcategories for FP category. #6308

Merged
merged 56 commits into from
Sep 22, 2023
Merged

i#6238: Add categorization of x86 instruction mix and subcategories for FP category. #6308

merged 56 commits into from
Sep 22, 2023

Conversation

kuhanov
Copy link
Contributor

@kuhanov kuhanov commented Sep 11, 2023 •
edited
Loading

This PR extends #6237 by adding categorization for x86 instruction mix. It introduces subcategories like MATH, CONVERT, and MOVE for both x86 and AArch64. For instance, arithmetic floating-point operations will have DR_INSTR_CATEGORY_FP | DR_INSTR_CATEGORY_MATH category.

Issue: #6238

Not all x86 opcodes are categorized nd such instructions are marked 'UNCATEGORIZED"

@kuhanov kuhanov self-assigned this Sep 11, 2023
@kuhanov kuhanov closed this Sep 11, 2023
@kuhanov kuhanov force-pushed the x86_instruction_categorization branch from 30f5e2c to d6cceb0 Compare September 11, 2023 11:08
@kuhanov kuhanov reopened this Sep 11, 2023
@kuhanov
Copy link
Contributor Author

kuhanov commented Sep 11, 2023

code_api|tool.drcachesim.scattergather-x86 test is failed without these changes.
I've tried master and the last cronbuild-10.0.19608
318: ASSERT FAILURE: ~/master/clients/drcachesim/tracer/output.cpp:505: towrite <= ipc_pipe.get_atomic_write_size() && towrite > 0 ()

towrite size is 4128 and ipc_pipe.get_atomic_write_size() is 4096 on the fail check
Thx, Kirill

@kuhanov
Copy link
Contributor Author

kuhanov commented Sep 12, 2023 •
edited
Loading

code_api|tool.drcachesim.scattergather-x86 test is failed without these changes. I've tried master and the last cronbuild-10.0.19608 318: ASSERT FAILURE: ~/master/clients/drcachesim/tracer/output.cpp:505: towrite <= ipc_pipe.get_atomic_write_size() && towrite > 0 ()

towrite size is 4128 and ipc_pipe.get_atomic_write_size() is 4096 on the fail check Thx, Kirill

Fail was started after #6290. Looks like you know about issue :)
"x86-64 failure is scattergather-x86 pipe assert #5329"

@derekbruening , could you review categorization for x86.
Thx, Kirill

@derekbruening
Copy link
Contributor

Was traveling. Will take a look.

@derekbruening derekbruening self-requested a review September 13, 2023 17:39
derekbruening
derekbruening reviewed Sep 14, 2023
View reviewed changes
Copy link
Contributor

@derekbruening derekbruening left a comment

Choose a reason for hiding this comment

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

Sorry, I don't have much time this week: only made it through the first 4 files so far but had some comments so sending them out.

core/ir/aarch64/codec.c Show resolved Hide resolved
core/ir/aarch64/codec.c Show resolved Hide resolved
core/ir/aarch64/codec.c Show resolved Hide resolved
core/ir/aarch64/codec.c Show resolved Hide resolved
core/ir/aarch64/codec.c Show resolved Hide resolved
core/ir/decode_api.h Outdated Show resolved Hide resolved
core/ir/decode_api.h Outdated Show resolved Hide resolved
core/ir/decode_api.h Outdated Show resolved Hide resolved
core/ir/instr_api.h Outdated Show resolved Hide resolved
core/ir/instr_api.h Outdated Show resolved Hide resolved
derekbruening
derekbruening reviewed Sep 14, 2023
View reviewed changes
core/ir/x86/decode.c Outdated Show resolved Hide resolved
core/ir/x86/decode.c Outdated Show resolved Hide resolved
core/ir/x86/decode_private.h Outdated Show resolved Hide resolved
core/ir/x86/decode_table.c Outdated Show resolved Hide resolved
@kuhanov kuhanov changed the title Add categorization of x86 instruction mix and subcotegories for FP category. Add categorization of x86 instruction mix and subcategories for FP category. Sep 15, 2023
@kuhanov kuhanov changed the title Add categorization of x86 instruction mix and subcategories for FP category. i#6238: Add categorization of x86 instruction mix and subcategories for FP category. Sep 15, 2023
@kuhanov kuhanov force-pushed the x86_instruction_categorization branch from 7ba6b51 to 1029ba5 Compare September 15, 2023 08:58
derekbruening
derekbruening reviewed Sep 19, 2023
View reviewed changes
Copy link
Contributor

@derekbruening derekbruening left a comment

Choose a reason for hiding this comment

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

Looking pretty good, but maybe worth one more glance after this set of suggestions before merging.

api/docs/release.dox Outdated Show resolved Hide resolved
core/ir/decode_api.h Outdated Show resolved Hide resolved
core/ir/instr_api.h Outdated Show resolved Hide resolved
core/ir/x86/decode.c Show resolved Hide resolved
core/ir/x86/decode_private.h Outdated Show resolved Hide resolved
core/ir/x86/decode.c Outdated Show resolved Hide resolved
core/ir/x86/decode.c Outdated Show resolved Hide resolved
core/ir/x86/decode.c Show resolved Hide resolved
core/ir/x86/decode.c Show resolved Hide resolved
core/ir/x86/decode.c Show resolved Hide resolved
derekbruening
derekbruening reviewed Sep 21, 2023
View reviewed changes
core/ir/x86/decode.c Outdated Show resolved Hide resolved
api/docs/release.dox Show resolved Hide resolved
derekbruening
derekbruening approved these changes Sep 22, 2023
View reviewed changes
Copy link
Contributor

@derekbruening derekbruening left a comment

Choose a reason for hiding this comment

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

Thank you for contributing this feature and working through the review process.

@kuhanov kuhanov merged commit d32cf34 into master Sep 22, 2023
@kuhanov kuhanov deleted the x86_instruction_categorization branch September 22, 2023 20:32
derekbruening added a commit that referenced this pull request Dec 9, 2024
@derekbruening
i#6238 op cat: Change fence opcode category to STATE
5398f63 
The x86 OP_*fence opcodes were marked with inaccurate categories in
the original PR #6308. Here we change all 3 to DR_INSTR_CATEGORY_STATE
under the logic that the STATE category is the one most likely to be
interpreted as a barrier by a simulator. (If we add an ATOMIC category
in the future we'll switch to that.)

Issue: #6238
@derekbruening derekbruening mentioned this pull request Dec 9, 2024
Merged
derekbruening added a commit that referenced this pull request Dec 9, 2024
@derekbruening
i#6238 op cat: Change fence opcode category to STATE (#7112)
2fe63a0 
The x86 OP_*fence opcodes were marked with inaccurate categories in the
original PR #6308. Here we change all 3 to DR_INSTR_CATEGORY_STATE under
the logic that the STATE category is the one most likely to be
interpreted as a barrier by a simulator. (If we add an ATOMIC category
in the future we'll switch to that.)

Issue: #6238
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AssadHashmi AssadHashmi AssadHashmi left review comments

@derekbruening derekbruening derekbruening approved these changes

Assignees

@kuhanov kuhanov

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kuhanov @derekbruening @AssadHashmi