gcc/gcc
gcc/gcc
Watch
43
Fork
You've already forked gcc
 58
Code Issues Pull requests 58 Wiki Activity Actions

Mathematically correct result from maybe_lt, maybe_le and derivatives (avoiding type traits) #180

Open
chris.bazley wants to merge 1 commit from chris.bazley/gcc:myforge/comparisons_b into trunk
pull from: chris.bazley/gcc:myforge/comparisons_b
merge into: gcc:trunk
Conversation 4 Commits 1 Files changed 2 +388 -25
chris.bazley commented 2026-06-18 15:59:02 +00:00
Member
Copy link

Most of the poly_int functions don't handle mixed signed/unsigned
parameter types correctly, but it is relatively easy to make
the comparison functions do so:

  1. If the first operand is negative and the second operand is
    positive or zero then a < b. This should optimise to nothing
    if the first operand is unsigned.
  2. If the first operand is positive or zero and the second operand
    is negative then a > b. This should optimise to nothing if the
    second operand is unsigned.
  3. Otherwise, both operands are positive or zero, or both operands
    are negative, so cast them to a common type then use the
    standard C++ comparison operator.

This approach is potentially less efficient than using type traits
because a < 0 and b < 0 are evaluated as well as a < b (or a <= b)
if both operands have signed types.

It would not be straightforward to make use of existing unsigned
test cases because they rely on implicit conversion of negative
integers to unsigned types, therefore I followed the following
method instead:

  1. Start with the test_signed_maybe_* and test_signed_known_* tests.
  2. Where a standalone coefficient is negative, use the signed
    coefficient type specified by the user of the template, otherwise
    the unsigned coefficient type.
  3. Delete all resulting test cases that involve standalone
    coefficients and do not have operands of types with different
    signedness.
  4. Duplicate the test cases that involve polynomial operands and
    modify them such that one version has the first operand of signed
    type and the other has the second operand of signed type.
  5. Update the coefficients of the polynomial operands to ensure that
    unsigned types have a non-negative value, trying to preserve intent,
    ordering, and relative distance between values.

Most of the poly_int functions don't handle mixed signed/unsigned
parameter types correctly, but it is relatively easy to make
the comparison functions do so:

  1. If both operand types are signed or both are unsigned, then use
    the standard C++ comparison operator.
  2. If the first operand's type is signed and the signedness of the
    two types mismatches (1) then the second operand's value is
    positive or zero, which is greater than the first operand's value
    if the latter is negative.
  3. If the second operand's type is signed and the signedness of the
    two types mismatches (1) then the first operand's value is
    positive or zero, which is greater than the second operand's value
    if the latter is negative.
  4. Otherwise, both operands are positive or zero, so cast them to a
    common type then use the standard C++ comparison operator.

Using type traits is likely to be more efficient than relying
on the fact that unsigned types never have a negative value,
because it avoids unnecessary comparisons in the case where
one or more of the operands has a signed type.

It would not be straightforward to make use of existing unsigned
test cases because they rely on implicit conversion of negative
integers to unsigned types, therefore I followed the following
method instead:

  1. Start with the test_signed_maybe_* and test_signed_known_* tests.
  2. Where a standalone coefficient is negative, use the signed
    coefficient type specified by the user of the template, otherwise
    the unsigned coefficient type.
  3. Delete all resulting test cases that involve standalone
    coefficients and do not have operands types with different
    signedness.
  4. Duplicate the test cases that involve polynomial operands and
    modify them such that one version has the first operand of signed
    type and the other has the second operand of signed type.
  5. Update the coefficients of the polynomial operands to ensure that
    unsigned types have a non-negative value, trying to preserve intent,
    ordering, and relative distance between values.

CC: rdsandiford@googlemail.com, tamar.christina@arm.com

Most of the poly_int functions don't handle mixed signed/unsigned parameter types correctly, but it is relatively easy to make the comparison functions do so: 1. If the first operand is negative and the second operand is positive or zero then a < b. This should optimise to nothing if the first operand is unsigned. 2. If the first operand is positive or zero and the second operand is negative then a > b. This should optimise to nothing if the second operand is unsigned. 3. Otherwise, both operands are positive or zero, or both operands are negative, so cast them to a common type then use the standard C++ comparison operator. This approach is potentially less efficient than using type traits because a < 0 and b < 0 are evaluated as well as a < b (or a <= b) if both operands have signed types. It would not be straightforward to make use of existing unsigned test cases because they rely on implicit conversion of negative integers to unsigned types, therefore I followed the following method instead: 1. Start with the test_signed_maybe_* and test_signed_known_* tests. 2. Where a standalone coefficient is negative, use the signed coefficient type specified by the user of the template, otherwise the unsigned coefficient type. 3. Delete all resulting test cases that involve standalone coefficients and do not have operands of types with different signedness. 4. Duplicate the test cases that involve polynomial operands and modify them such that one version has the first operand of signed type and the other has the second operand of signed type. 5. Update the coefficients of the polynomial operands to ensure that unsigned types have a non-negative value, trying to preserve intent, ordering, and relative distance between values. Most of the poly_int functions don't handle mixed signed/unsigned parameter types correctly, but it is relatively easy to make the comparison functions do so: 1. If both operand types are signed or both are unsigned, then use the standard C++ comparison operator. 2. If the first operand's type is signed and the signedness of the two types mismatches (1) then the second operand's value is positive or zero, which is greater than the first operand's value if the latter is negative. 3. If the second operand's type is signed and the signedness of the two types mismatches (1) then the first operand's value is positive or zero, which is greater than the second operand's value if the latter is negative. 4. Otherwise, both operands are positive or zero, so cast them to a common type then use the standard C++ comparison operator. Using type traits is likely to be more efficient than relying on the fact that unsigned types never have a negative value, because it avoids unnecessary comparisons in the case where one or more of the operands has a signed type. It would not be straightforward to make use of existing unsigned test cases because they rely on implicit conversion of negative integers to unsigned types, therefore I followed the following method instead: 1. Start with the test_signed_maybe_* and test_signed_known_* tests. 2. Where a standalone coefficient is negative, use the signed coefficient type specified by the user of the template, otherwise the unsigned coefficient type. 3. Delete all resulting test cases that involve standalone coefficients and do not have operands types with different signedness. 4. Duplicate the test cases that involve polynomial operands and modify them such that one version has the first operand of signed type and the other has the second operand of signed type. 5. Update the coefficients of the polynomial operands to ensure that unsigned types have a non-negative value, trying to preserve intent, ordering, and relative distance between values. CC: rdsandiford@googlemail.com, tamar.christina@arm.com
forge-bot commented 2026-06-18 15:59:03 +00:00
Member
Copy link

Hi @chris.bazley. This bot helps send your PR as a patch series to the mailing list.

Next steps

  1. Write a clear PR description. It will be used as the cover letter.
  2. To CC extra people, add lines like this at the end of the PR description: CC: Reviewer1 <revi.ewer@example.com>, Reviewer2 <an.other@example.com>.
  3. To preview the emails, comment with /preview.
  4. When ready, request review in the Reviewers section. Or, if you just want to send the series, comment with /submit.
  5. Use the cover-letter links below to follow replies.
  6. For a new patch series, push your changes, update the PR description with a Changes since vX section, and comment with /submit again.

See Submitting Patches and UsingForge.

Consider joining the gcc and gcc-patches mailing lists..
For real time communication, check the gcc irc channels.

Or join #overseers on Libera Chat, particularly if this automation is not working (stay online to get replies, IRC does not save messages if people are not online).

<!-- pr-welcome --> Hi @chris.bazley. This bot helps send your PR as a patch series to the mailing list. ## Next steps 1. Write a clear PR description. It will be used as the cover letter. 2. To CC extra people, add lines like this **at the end** of the PR description: `CC: Reviewer1 <revi.ewer@example.com>, Reviewer2 <an.other@example.com>`. 3. To preview the emails, comment with `/preview`. 4. When ready, request review in the Reviewers section. Or, if you just want to send the series, comment with `/submit`. 5. Use the cover-letter links below to follow replies. 6. For a new patch series, push your changes, update the PR description with a `Changes since vX` section, and comment with `/submit` again. See [Submitting Patches](https://gcc.gnu.org/contribute.html#patches) and [UsingForge](https://gcc.gnu.org/wiki/UsingForge). Consider joining the [gcc and gcc-patches mailing lists.](https://gcc.gnu.org/lists.html). For real time communication, check the [gcc irc channels](https://gcc.gnu.org/wiki/GCConIRC). Or join [`#overseers`](https://web.libera.chat/#overseers) on Libera Chat, particularly if this automation is not working (stay online to get replies, IRC does not save messages if people are not online).
chris.bazley commented 2026-06-18 16:01:00 +00:00
Author
Member
Copy link

I used these functions as part of my work on gcc/gcc-TEST#176 and found they don't work for mixed signed / unsigned operand types. I think it might be worth fixing that to prevent future surprises.

This approach is an alternative to the one that I used in gcc/gcc-TEST#179 because I was not sure which was more likely to be accepted. The tests are identical. Please state your preference, if any.

I used these functions as part of my work on https://forge.sourceware.org/gcc/gcc-TEST/pulls/176 and found they don't work for mixed signed / unsigned operand types. I think it might be worth fixing that to prevent future surprises. This approach is an alternative to the one that I used in https://forge.sourceware.org/gcc/gcc-TEST/pulls/179 because I was not sure which was more likely to be accepted. The tests are identical. Please state your preference, if any.
chris.bazley force-pushed myforge/comparisons_b from 1ee378e31f
Some checks failed
/ format-checks (pull_request) Failing after 1m37s
Details
to c6a3ddbb71
All checks were successful
/ format-checks (pull_request) Successful in 1m33s
Details
2026-06-18 16:13:40 +00:00 Compare
chris.bazley force-pushed myforge/comparisons_b from c6a3ddbb71
All checks were successful
/ format-checks (pull_request) Successful in 1m33s
Details
to da888bbaed
All checks were successful
/ format-checks (pull_request) Successful in 1m30s
Details
2026-06-18 16:15:51 +00:00 Compare
chris.bazley force-pushed myforge/comparisons_b from da888bbaed
All checks were successful
/ format-checks (pull_request) Successful in 1m30s
Details
to 4cc72b71ba
All checks were successful
/ format-checks (pull_request) Successful in 1m37s
Details
2026-06-18 16:27:10 +00:00 Compare
rsandifo commented 2026-06-18 19:41:06 +00:00
Member
Copy link

@chris.bazley wrote in gcc/gcc-TEST#180 (comment):

I used these functions as part of my work on #176 and found they don't work for mixed signed / unsigned operand types. I think it might be worth fixing that to prevent future surprises.

This approach is an alternative to the one that I used in #179 because I was not sure which was more likely to be accepted. The tests are identical. Please state your preference, if any.

As I mentioned in #176, I think it's a feature that known_lt gives a warning for mismatched signs, since < does the same.

I think it's ok for mixed-signedness divisions to trigger a warning as well. That's stricter than C++, but it would force callers to make a choice. The C++ behaviour of silently converting int64_t / uint64_t to uint64_t / uint64_t doesn't seem something to cherish.

If we did want to make can_div_away_from_zero_p behave like C++ division, i.e. convert to the common type before dividing, then the way to do that would be with a cast to POLY_POLY_RESULT, or similar. But like I say, I think we should avoid that. It just seems like setting a trap for the unwary.

So I still prefer the version I suggested above. There again, if you did go with that, I'd no longer be able to approve the patch :-)

@chris.bazley wrote in https://forge.sourceware.org/gcc/gcc-TEST/pulls/180#issuecomment-6379: > I used these functions as part of my work on #176 and found they don't work for mixed signed / unsigned operand types. I think it might be worth fixing that to prevent future surprises. > > This approach is an alternative to the one that I used in #179 because I was not sure which was more likely to be accepted. The tests are identical. Please state your preference, if any. As I mentioned in #176, I think it's a feature that `known_lt` gives a warning for mismatched signs, since `<` does the same. I think it's ok for mixed-signedness divisions to trigger a warning as well. That's stricter than C++, but it would force callers to make a choice. The C++ behaviour of silently converting `int64_t / uint64_t` to `uint64_t / uint64_t` doesn't seem something to cherish. If we did want to make `can_div_away_from_zero_p` behave like C++ division, i.e. convert to the common type *before* dividing, then the way to do that would be with a cast to `POLY_POLY_RESULT`, or similar. But like I say, I think we should avoid that. It just seems like setting a trap for the unwary. So I still prefer the version I suggested above. There again, if you did go with that, I'd no longer be able to approve the patch :-)
chris.bazley commented 2026-06-19 10:16:35 +00:00
Author
Member
Copy link

@rsandifo wrote in gcc/gcc-TEST#180 (comment):

@chris.bazley wrote in #180 (comment):

I used these functions as part of my work on #176 and found they don't work for mixed signed / unsigned operand types. I think it might be worth fixing that to prevent future surprises.
This approach is an alternative to the one that I used in #179 because I was not sure which was more likely to be accepted. The tests are identical. Please state your preference, if any.

As I mentioned in #176, I think it's a feature that known_lt gives a warning for mismatched signs, since < does the same.

I think it's ok for mixed-signedness divisions to trigger a warning as well. That's stricter than C++, but it would force callers to make a choice. The C++ behaviour of silently converting int64_t / uint64_t to uint64_t / uint64_t doesn't seem something to cherish.

If we did want to make can_div_away_from_zero_p behave like C++ division, i.e. convert to the common type before dividing, then the way to do that would be with a cast to POLY_POLY_RESULT, or similar. But like I say, I think we should avoid that. It just seems like setting a trap for the unwary.

So I still prefer the version I suggested above. There again, if you did go with that, I'd no longer be able to approve the patch :-)

I can see both sides of the argument. These comparison macros which produce a mathematically correct result are very likely to be part of C2Y: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3776.pdf

My comments here, on a related proposal for standard min/max macros, also seem relevant:
https://open-std.org/jtc1/sc22/wg14/35823
https://open-std.org/jtc1/sc22/wg14/36744

I feel more strongly against functions that potentially return an unexpected result that is a number, than functions that simply return a Boolean answer (as in the case of maybe_lt). I'm not sure there's any rational reason for that though.

tl;dr: I have argued against the thing I have implemented in this patch, but the wind seems to be blowing in the opposite direction.

@rsandifo wrote in https://forge.sourceware.org/gcc/gcc-TEST/pulls/180#issuecomment-6403: > @chris.bazley wrote in #180 (comment): > > > I used these functions as part of my work on #176 and found they don't work for mixed signed / unsigned operand types. I think it might be worth fixing that to prevent future surprises. > > This approach is an alternative to the one that I used in #179 because I was not sure which was more likely to be accepted. The tests are identical. Please state your preference, if any. > > As I mentioned in #176, I think it's a feature that `known_lt` gives a warning for mismatched signs, since `<` does the same. > > I think it's ok for mixed-signedness divisions to trigger a warning as well. That's stricter than C++, but it would force callers to make a choice. The C++ behaviour of silently converting `int64_t / uint64_t` to `uint64_t / uint64_t` doesn't seem something to cherish. > > If we did want to make `can_div_away_from_zero_p` behave like C++ division, i.e. convert to the common type _before_ dividing, then the way to do that would be with a cast to `POLY_POLY_RESULT`, or similar. But like I say, I think we should avoid that. It just seems like setting a trap for the unwary. > > So I still prefer the version I suggested above. There again, if you did go with that, I'd no longer be able to approve the patch :-) I can see both sides of the argument. These comparison macros which produce a mathematically correct result are very likely to be part of C2Y: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3776.pdf My comments here, on a related proposal for standard min/max macros, also seem relevant: https://open-std.org/jtc1/sc22/wg14/35823 https://open-std.org/jtc1/sc22/wg14/36744 I feel more strongly against functions that potentially return an unexpected result that is a number, than functions that simply return a Boolean answer (as in the case of maybe_lt). I'm not sure there's any rational reason for that though. tl;dr: I have argued against the thing I have implemented in this patch, but the wind seems to be blowing in the opposite direction.
chris.bazley changed title from Mathematically correct result from maybe_lt, maybe_le and derivatives to Mathematically correct result from maybe_lt, maybe_le and derivatives (avoiding type traits) 2026-06-19 11:56:56 +00:00
All checks were successful
/ format-checks (pull_request) Successful in 1m37s
Details
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u myforge/comparisons_b:chris.bazley-myforge/comparisons_b
git switch chris.bazley-myforge/comparisons_b

Merge

Merge the changes and update on Forgejo.
git switch trunk
git merge --no-ff chris.bazley-myforge/comparisons_b
git switch chris.bazley-myforge/comparisons_b
git rebase trunk
git switch trunk
git merge --ff-only chris.bazley-myforge/comparisons_b
git switch chris.bazley-myforge/comparisons_b
git rebase trunk
git switch trunk
git merge --no-ff chris.bazley-myforge/comparisons_b
git switch trunk
git merge --squash chris.bazley-myforge/comparisons_b
git switch trunk
git merge --ff-only chris.bazley-myforge/comparisons_b
git switch trunk
git merge chris.bazley-myforge/comparisons_b
git push origin trunk
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
Compat/Breaking
Breaking change that won't be backward compatible

  
Frontend/ada

   
Frontend/c

   
Frontend/c++

   
Frontend/rust
Code affecting the Rust front end

  
General/forge
Forgejo infrastructure

  
Kind/Bug
Something is not working

  
Kind/Documentation
Documentation changes

  
Kind/Enhancement
Improve existing functionality

  
Kind/Feature
New functionality

  
Kind/Security
This is security issue

  
Kind/Testing
Issue or pull request related to testing

  
Library/libgcc

   
Library/libstdc++

   
Midend/gimple

   
Midend/rtl

   
Midend/tree
  
Priority
Critical
 The priority is critical

  
Priority
High
 The priority is high

  
Priority
Low
 The priority is low

  
Priority
Medium
 The priority is medium

  
Reviewed
Confirmed
 Issue has been confirmed

  
Reviewed
Duplicate
 This issue or pull request already exists

  
Reviewed
Invalid
 Invalid issue

  
Reviewed
Won't Fix
 This issue won't be fixed

  
Status
Abandoned
 Somebody has started to work on this but abandoned work

  
Status
Blocked
 Something is blocking this issue or pull request

  
Status
Need More Info
 Feedback is required to reproduce issue or to continue work

  
Target/aarch64

   
Target/arm

   
Target/i386
  
bug
Something is not working

  
duplicate
This issue or pull request already exists

  
enhancement
New feature

  
help wanted
Need some help

  
invalid
Something is wrong

  
question
More information is needed

  
wontfix
This won't be fixed

No labels
Compat/Breaking
Frontend/ada
Frontend/c
Frontend/c++
Frontend/rust
General/forge
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Library/libgcc
Library/libstdc++
Midend/gimple
Midend/rtl
Midend/tree
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Target/aarch64
Target/arm
Target/i386
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gcc/gcc!180
No description provided.