Skip to content

Float CmpResult seems to be of the wrong type #919

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.

Sign up for GitHub

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

Jump to bottom
Open
antoyo opened this issue May 22, 2025 · 9 comments · May be fixed by #920
Open

Float CmpResult seems to be of the wrong type #919

antoyo opened this issue May 22, 2025 · 9 comments · May be fixed by #920

Comments

@antoyo
Copy link

antoyo commented May 22, 2025 •
edited
Loading

Hi.
I'm currently trying to make some new f128 tests pass for cg_gcc and I believe I found out the issue: it seems the CmpResult type is aliased wrongly to i32 while it should be aliased to i64 (or at least long) on x86-64.

The type is defined here as a i32 while the comment just above point to this that mentions that it should be a long.

Could you please double-check that it indeed should be a long and not a i32?

It seems like it should be a int on Aarch64, according to the LLVM code linked above.

Thanks.
@tgross35
Copy link
Contributor

That's interesting, agreed that it should be a long. I'll update to match LLVM's logic here. I tried to double check against GCC but the exact logic is a bit hidden behind the __libgcc_cmp_return__ mode and I have no idea where that is actually getting set https://github.com/search?q=repo%3Agcc-mirror%2Fgcc+%2Flibgcc_cmp_return_mode%2F+path%3Agcc&type=code.

Out of curiosity, how did you notice the difference here? I guess if LLVM only reads the lower bits of the return but GCC checks the whole register, that would be visible.

@antoyo
Copy link
Author

antoyo commented May 23, 2025 •
edited
Loading

Out of curiosity, how did you notice the difference here? I guess if LLVM only reads the lower bits of the return but GCC checks the whole register, that would be visible.

In cg_gcc, I saw the following instruction to check the result:

test    rax, rax

while cg_llvm had:

test    eax, eax

I tried to double check against GCC but the exact logic is a bit hidden behind the __libgcc_cmp_return__ mode and I have no idea where that is actually getting set

From a quick glance, this is a target hook that can be overwritten by the backend (see here for Aarch64 for instance).
The default value seems to be defined here.

Thanks!

tgross35 added a commit to tgross35/compiler-builtins that referenced this issue May 23, 2025
@tgross35
Update CmpResult to use a pointer-sized return type
9587d56 
As seen at [1], LLVM uses `long long` on LLP64 (to get a 64-bit integer
matching pointer size) and `long` on everything else, with exceptions
for AArch64 and AVR. Our current logic always uses an `i32`. This
happens to work because LLVM uses 32-bit instructions to check the
output on x86-64, but the GCC checks the full 64-bit register so garbage
in the upper half leads to incorrect results.

Update our return type to be `isize`, with exceptions for AArch64 and
AVR.

Fixes: rust-lang#919

[1]: https://github.com/llvm/llvm-project/blob/0cf3c437c18ed27d9663d87804a9a15ff6874af2/compiler-rt/lib/builtins/fp_compare_impl.inc#L11-L27
@tgross35 tgross35 linked a pull request May 23, 2025 that will close this issue
Open
@tgross35
Copy link
Contributor

Oh, so I guess my searches just didn't find anything because GH doesn't index the 30k line GCC target files :)

Posted a fix in #920, would you mind reviewing?

tgross35 added a commit to tgross35/compiler-builtins that referenced this issue May 23, 2025
@tgross35
Update CmpResult to use a pointer-sized return type
3be0f11 
As seen at [1], LLVM uses `long long` on LLP64 (to get a 64-bit integer
matching pointer size) and `long` on everything else, with exceptions
for AArch64 and AVR. Our current logic always uses an `i32`. This
happens to work because LLVM uses 32-bit instructions to check the
output on x86-64, but the GCC checks the full 64-bit register so garbage
in the upper half leads to incorrect results.

Update our return type to be `isize`, with exceptions for AArch64 and
AVR.

Fixes: rust-lang#919

[1]: https://github.com/llvm/llvm-project/blob/0cf3c437c18ed27d9663d87804a9a15ff6874af2/compiler-rt/lib/builtins/fp_compare_impl.inc#L11-L27
@beetrees
Copy link
Contributor

Inconsistent with the compilers, the GCC docs list int as the return type for the comparison functions.

@tgross35
Copy link
Contributor

tgross35 commented May 23, 2025 •
edited
Loading

For completeness, I checked for other overrides. Doesn't seem like anything other than aarch64 uses anything different:

// src/gcc/gcc/targhooks.cc
scalar_int_mode
default_libgcc_cmp_return_mode (void)
{
  return word_mode;
}
// src/gcc/gcc/config/aarch64/aarch64.cc
#undef TARGET_LIBGCC_CMP_RETURN_MODE
#define TARGET_LIBGCC_CMP_RETURN_MODE aarch64_libgcc_cmp_return_mode

static scalar_int_mode
aarch64_libgcc_cmp_return_mode (void)
{
  return SImode;
}
// src/gcc/gcc/config/rs6000/rs6000.cc
#undef TARGET_LIBGCC_CMP_RETURN_MODE
#define TARGET_LIBGCC_CMP_RETURN_MODE rs6000_abi_word_mode

static scalar_int_mode
rs6000_abi_word_mode (void)
{
  return TARGET_32BIT ? SImode : DImode;
}
// src/gcc/gcc/config/s390/s390.cc
#undef TARGET_LIBGCC_CMP_RETURN_MODE
#define TARGET_LIBGCC_CMP_RETURN_MODE s390_libgcc_cmp_return_mode

static scalar_int_mode
s390_libgcc_cmp_return_mode (void)
{
  return TARGET_64BIT ? DImode : SImode;
}
// libgcc/config/avr/avr-lib.h

#ifdef FLOAT
#define CMPtype QItype

tgross35 added a commit to tgross35/compiler-builtins that referenced this issue May 23, 2025
@tgross35
Update CmpResult to use a pointer-sized return type
bff4fe1 
As seen at [1], LLVM uses `long long` on LLP64 (to get a 64-bit integer
matching pointer size) and `long` on everything else, with exceptions
for AArch64 and AVR. Our current logic always uses an `i32`. This
happens to work because LLVM uses 32-bit instructions to check the
output on x86-64, but the GCC checks the full 64-bit register so garbage
in the upper half leads to incorrect results.

Update our return type to be `isize`, with exceptions for AArch64 and
AVR.

Fixes: rust-lang#919

[1]: https://github.com/llvm/llvm-project/blob/0cf3c437c18ed27d9663d87804a9a15ff6874af2/compiler-rt/lib/builtins/fp_compare_impl.inc#L11-L27
tgross35 added a commit to tgross35/compiler-builtins that referenced this issue May 23, 2025
@tgross35
Update CmpResult to use a pointer-sized return type
f4ec382 
As seen at [1], LLVM uses `long long` on LLP64 (to get a 64-bit integer
matching pointer size) and `long` on everything else, with exceptions
for AArch64 and AVR. Our current logic always uses an `i32`. This
happens to work because LLVM uses 32-bit instructions to check the
output on x86-64, but the GCC checks the full 64-bit register so garbage
in the upper half leads to incorrect results.

Update our return type to be `isize`, with exceptions for AArch64 and
AVR.

Fixes: rust-lang#919

[1]: https://github.com/llvm/llvm-project/blob/0cf3c437c18ed27d9663d87804a9a15ff6874af2/compiler-rt/lib/builtins/fp_compare_impl.inc#L11-L27
tgross35 added a commit to tgross35/compiler-builtins that referenced this issue May 23, 2025
@tgross35
Update CmpResult to use a pointer-sized return type
2459c35 
As seen at [1], LLVM uses `long long` on LLP64 (to get a 64-bit integer
matching pointer size) and `long` on everything else, with exceptions
for AArch64 and AVR. Our current logic always uses an `i32`. This
happens to work because LLVM uses 32-bit instructions to check the
output on x86-64, but the GCC checks the full 64-bit register so garbage
in the upper half leads to incorrect results.

Update our return type to be `isize`, with exceptions for AArch64 and
AVR.

Fixes: rust-lang#919

[1]: https://github.com/llvm/llvm-project/blob/0cf3c437c18ed27d9663d87804a9a15ff6874af2/compiler-rt/lib/builtins/fp_compare_impl.inc#L11-L27
tgross35 added a commit to tgross35/compiler-builtins that referenced this issue May 23, 2025
@tgross35
Update CmpResult to use a pointer-sized return type
a100507 
As seen at [1], LLVM uses `long long` on LLP64 (to get a 64-bit integer
matching pointer size) and `long` on everything else, with exceptions
for AArch64 and AVR. Our current logic always uses an `i32`. This
happens to work because LLVM uses 32-bit instructions to check the
output on x86-64, but the GCC checks the full 64-bit register so garbage
in the upper half leads to incorrect results.

Update our return type to be `isize`, with exceptions for AArch64 and
AVR.

Fixes: rust-lang#919

[1]: https://github.com/llvm/llvm-project/blob/0cf3c437c18ed27d9663d87804a9a15ff6874af2/compiler-rt/lib/builtins/fp_compare_impl.inc#L11-L27
tgross35 added a commit to tgross35/compiler-builtins that referenced this issue May 23, 2025
@tgross35
Update CmpResult to use a pointer-sized return type
e051061 
As seen at [1], LLVM uses `long long` on LLP64 (to get a 64-bit integer
matching pointer size) and `long` on everything else, with exceptions
for AArch64 and AVR. Our current logic always uses an `i32`. This
happens to work because LLVM uses 32-bit instructions to check the
output on x86-64, but the GCC checks the full 64-bit register so garbage
in the upper half leads to incorrect results.

Update our return type to be `isize`, with exceptions for AArch64 and
AVR.

Fixes: rust-lang#919

[1]: https://github.com/llvm/llvm-project/blob/0cf3c437c18ed27d9663d87804a9a15ff6874af2/compiler-rt/lib/builtins/fp_compare_impl.inc#L11-L27
@antoyo
Copy link
Author

antoyo commented May 23, 2025

Posted a fix in #920, would you mind reviewing?

Yes, I'll test this with cg_gcc when the CI passes.

@tgross35
Copy link
Contributor

Inconsistent with the compilers, the GCC docs list int as the return type for the comparison functions.

Submitted https://gcc.gnu.org/pipermail/gcc-patches/2025-May/684626.html

@tgross35
Copy link
Contributor

Posted a fix in #920, would you mind reviewing?

Yes, I'll test this with cg_gcc when the CI passes.

I think you should be able to test it whenever, the remaining failures are testing against the version we’re currently shipping (I don’t think I’ll get to that before tomorrow)

@antoyo
Copy link
Author

antoyo commented May 24, 2025

I just ran the core tests and they now pass with this fix.
Thanks for fixing this quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Update CmpResult to use a pointer-sized return type tgross35/compiler-builtins
3 participants
@antoyo @tgross35 @beetrees