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.

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

extend the computeOverflowForSignedSub/computeOverflowForUnsignedSub implementations with ConstantRange, and add tests #67724

Closed
wants to merge 1 commit into from

Conversation

elhewaty
Copy link
Contributor

No description provided.

; AVX-NEXT: retq
%x = call { <4 x i8>, <4 x i1> } @llvm.usub.with.overflow.v4i8(<4 x i8> <i8 255, i8 255, i8 255, i8 255>, <4 x i8> <i8 128, i8 0, i8 255, i8 1>)
ret { <4 x i8>, <4 x i1> } %x
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please can you pre-commit these (or would you like me to?) so this patch shows the codegen diff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will but can you tell me what are the expectations and how can I see the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Give me just a small example of what should the code do, so that I will have more understanding of the problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pulled the tests from InstCombine/with_overfow.ll
but the difference between the two commits is so small, just one or two assembly lines. Is that even possible?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What we need to do is show the codegen diff from the patch - so ideally this patch needs to be split into a stack of 2 commits - the first just adds the new tests with the current codegen, then the second commit, with the computeOverflowFor*Sub implementations, shows the change in the codegen for the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand that, I mean what do computeOverflowFor*Sub functions do?
What optimizations they perform?
I will split the pull now.

@elhewaty
Copy link
Contributor Author

I will create a new pull request, I think I messed up everything here.

@elhewaty elhewaty closed this Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:X86 llvm:SelectionDAG SelectionDAGISel as well
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants