Skip to content

Conversation

@jargh
Copy link
Contributor

@jargh jargh commented Mar 21, 2025

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jargh jargh marked this pull request as ready for review March 31, 2025 17:20
@jargh jargh requested a review from pennyannn March 31, 2025 17:24
// MOVI
if q then
// MOVI, 128-bit only
if cmode = word 0b1110 /\ q then
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change looks fine to me. This additional guard is necessary because arm_adv_simd_expand_imm can now handle more cases.

Copy link
Collaborator

@pennyannn pennyannn left a comment

Choose a reason for hiding this comment

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

I have done the following things:

  1. Checked the modeling against ARM manual
  2. Running this PR on my graviton3 machine and I found all modeled instruction variants in the output

@pennyannn
Copy link
Collaborator

Should these instructions be added to allowed_asm?

This should fix issues pointed out in the code review. The
arm/allowed_asm list has been updated for other recently
added instructions too.
@jargh
Copy link
Contributor Author

jargh commented Apr 1, 2025

Thanks for the eagle-eyed review! I think the new commit fixes all that. I took the chance to add other new instructions that were added recently to the allowed_asm list, even though many of them are not yet used in the code, or used only in some branches.

Copy link
Collaborator

@aqjune-aws aqjune-aws left a comment

Choose a reason for hiding this comment

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

  • Checked that the bit formats are equal to what the Arm doc describes.
  • Also checked that cosimulator does not fail for > 10 minutes, after excluding all scalar instructions to focus on possibly affected vector instructions.

@aqjune-aws
Copy link
Collaborator

Probably we can merge after Nevine's patch is merged first, since her PR is reviewed first..?

@aqjune-aws aqjune-aws merged commit 1fcb664 into awslabs:main Apr 3, 2025
6 checks passed
@jargh jargh deleted the rej branch April 3, 2025 03:32
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.

3 participants