[HIP] Test signed 23-bit division and remainder#423
Open
LU-JOHN wants to merge 2 commits into
Open
Conversation
Signed-off-by: John Lu <John.Lu@amd.com>
There was a problem hiding this comment.
Pull request overview
Adds a new HIP test to exercise signed 23-bit division/remainder across a broad operand set (including the important NEG_MAX / -1 case) to demonstrate the need for the linked LLVM fix.
Changes:
- Add new HIP test
sdivrem23.hipthat computes and validates signed 23-bit div/rem results on device vs host. - Add
sdivrem23.reference_outputto integrate the test into the suite’s verification flow. - Register the new test in
External/HIP/CMakeLists.txt.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| External/HIP/sdivrem23.hip | New signed 23-bit div/rem kernel + host-side input generation and result verification. |
| External/HIP/sdivrem23.reference_output | Expected “PASSED!” output for the new test. |
| External/HIP/CMakeLists.txt | Adds sdivrem23 to the HIP local test list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: John Lu <John.Lu@amd.com>
Contributor
|
Does the test work without the LLVM patch you are referencing or would it miscompile? |
Contributor
Author
It will miscompile without llvm/llvm-project#203436. |
LU-JOHN
added a commit
to llvm/llvm-project
that referenced
this pull request
Jun 17, 2026
When expanding div/rem by using floating-point operations, sign/zero-extending the result from the calculated DivBits input width to 32-bits is unnecessary. CreateFPToSI or CreateFPToUI is called with a 32-bit int type so the conversion instruction will already produce a result with the desired width. Also it is incorrect. For signed-division `DIVBITS_MAX_NEG/-1`, the result should be `-DIVBITS_MAX_NEG` a positive value. Sign-extension will incorrectly return a negative result. For example, for DivBits=4, `-8/-1 = 8`, but adding code to do a 28-bit sign-extension will incorrectly return `-8`. Tested in llvm/llvm-test-suite#423. --------- Signed-off-by: John Lu <John.Lu@amd.com>
llvm-upstreamsync Bot
pushed a commit
to qualcomm/cpullvm-toolchain
that referenced
this pull request
Jun 17, 2026
… (#203436) When expanding div/rem by using floating-point operations, sign/zero-extending the result from the calculated DivBits input width to 32-bits is unnecessary. CreateFPToSI or CreateFPToUI is called with a 32-bit int type so the conversion instruction will already produce a result with the desired width. Also it is incorrect. For signed-division `DIVBITS_MAX_NEG/-1`, the result should be `-DIVBITS_MAX_NEG` a positive value. Sign-extension will incorrectly return a negative result. For example, for DivBits=4, `-8/-1 = 8`, but adding code to do a 28-bit sign-extension will incorrectly return `-8`. Tested in llvm/llvm-test-suite#423. --------- Signed-off-by: John Lu <John.Lu@amd.com>
llvm-sync Bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jun 17, 2026
… (#203436) When expanding div/rem by using floating-point operations, sign/zero-extending the result from the calculated DivBits input width to 32-bits is unnecessary. CreateFPToSI or CreateFPToUI is called with a 32-bit int type so the conversion instruction will already produce a result with the desired width. Also it is incorrect. For signed-division `DIVBITS_MAX_NEG/-1`, the result should be `-DIVBITS_MAX_NEG` a positive value. Sign-extension will incorrectly return a negative result. For example, for DivBits=4, `-8/-1 = 8`, but adding code to do a 28-bit sign-extension will incorrectly return `-8`. Tested in llvm/llvm-test-suite#423. --------- Signed-off-by: John Lu <John.Lu@amd.com>
nasherm
pushed a commit
to nasherm/llvm-project
that referenced
this pull request
Jun 18, 2026
When expanding div/rem by using floating-point operations, sign/zero-extending the result from the calculated DivBits input width to 32-bits is unnecessary. CreateFPToSI or CreateFPToUI is called with a 32-bit int type so the conversion instruction will already produce a result with the desired width. Also it is incorrect. For signed-division `DIVBITS_MAX_NEG/-1`, the result should be `-DIVBITS_MAX_NEG` a positive value. Sign-extension will incorrectly return a negative result. For example, for DivBits=4, `-8/-1 = 8`, but adding code to do a 28-bit sign-extension will incorrectly return `-8`. Tested in llvm/llvm-test-suite#423. --------- Signed-off-by: John Lu <John.Lu@amd.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test signed 23-bit division and remainder with a variety of values. Specifically, 23-bit NEG_MAX divided by -1 is tested. This shows why llvm/llvm-project#203436 is needed.