Skip to content

x64: Improve codegen for vectors with constant shift amounts#5797

Merged
alexcrichton merged 1 commit intobytecodealliance:mainfrom
alexcrichton:improve-x64-constant-vec-shifts
Feb 16, 2023
Merged

x64: Improve codegen for vectors with constant shift amounts#5797
alexcrichton merged 1 commit intobytecodealliance:mainfrom
alexcrichton:improve-x64-constant-vec-shifts

Conversation

@alexcrichton
Copy link
Member

I stumbled across this working on #5795 and figured this was a nice opportunity to improve the codegen here.

Comment on lines +490 to +523
; addb %al, (%rax)
; addb %al, (%rax)
; addb %al, (%rax)
; addb %al, (%rax)
; addb %al, (%rax)
; jg 0xb1
; jg 0xb3
; jg 0xb5
; jg 0xb7
; jg 0xb9
; jg 0xbb
; jg 0xbd
; jg 0xbf
; addb %al, (%rcx)
; addb (%rbx), %al
; addb $5, %al
Copy link
Member Author

Choose a reason for hiding this comment

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

I'll admit I don't know what's happening here. This I believe is the constant pool area and capstone is trying to decode it as instructions, but I don't know why after this change, which should generate a smaller constant pool, is generating more instructions here. I would hazard a guess that capstone automatically stoped beforehand for some reason and now it's keeping-on-going, but I'm not sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is at least partially the same issue that @elliottt is tackling in #5798. We've discussed ways to entirely quit trying to disassemble constants since we think the machbuffer knows where they are, but one step at a time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since #5798 is merged, can you try rebasing and see if the filetests look any better now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, I see you already did and it looks like it didn't help. I'm going to declare that this is harmless and move on with reviewing the rest of this PR.

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:x64 Issues related to x64 codegen isle Related to the ISLE domain-specific language labels Feb 16, 2023
@github-actions
Copy link

Subscribe to Label Action

cc @cfallin, @fitzgen

Details This issue or pull request has been labeled: "cranelift", "cranelift:area:x64", "isle"

Thus the following users have been cc'd because of the following labels:

  • cfallin: isle
  • fitzgen: isle

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@alexcrichton alexcrichton force-pushed the improve-x64-constant-vec-shifts branch from 3749ef2 to e73a99b Compare February 16, 2023 15:33
Copy link
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

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

Yup, this looks right to me. Thanks!

I stumbled across this working on bytecodealliance#5795 and figured this was a nice
opportunity to improve the codegen here.
@alexcrichton alexcrichton force-pushed the improve-x64-constant-vec-shifts branch from e73a99b to b5265ab Compare February 16, 2023 19:57
@alexcrichton alexcrichton added this pull request to the merge queue Feb 16, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 16, 2023
@alexcrichton alexcrichton added this pull request to the merge queue Feb 16, 2023
Merged via the queue into bytecodealliance:main with commit cae3b26 Feb 16, 2023
@alexcrichton alexcrichton deleted the improve-x64-constant-vec-shifts branch February 16, 2023 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cranelift:area:x64 Issues related to x64 codegen cranelift Issues related to the Cranelift code generator isle Related to the ISLE domain-specific language

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants