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

Add ctpop intrinsics #3246

Closed
wants to merge 1 commit into from
Closed

Add ctpop intrinsics #3246

wants to merge 1 commit into from

Conversation

c8ef
Copy link

@c8ef c8ef commented Nov 10, 2024

Part of #658.

This patch adds support for the ctpop intrinsic.

gcc/rust/ChangeLog:

    * backend/rust-compile-intrinsic.cc (ctpop_hander):

gcc/testsuite/ChangeLog:

    * rust/execute/torture/ctpop.rs: New test.

Signed-off-by: Yuao Ma [email protected]

@c8ef c8ef marked this pull request as ready for review November 10, 2024 05:13
@c8ef
Copy link
Author

c8ef commented Nov 10, 2024

Dear reviewers, the CI result looks good overall except for the changelog check. If the patch looks good, I will update the commit message accordingly. Thank you!

CC @philberty @CohenArthur

@dkm
Copy link
Member

dkm commented Nov 10, 2024

Hello!
Thanks for your contribution!

As we are contributing everything to GCC, we need to follow their rules wrt licensing/authorship. Can you please read https://gcc.gnu.org/dco.html and make sure you add the necessary Signed-off-by line in your commit log and clearly state in your PR description that you've read and agree to the conditions ? Unless you already have an entry in the dco file in upstream GCC or have filed the needed papers for the copyright assignment to the FSF.

Also, we'd need you to squash all 3 commits in a single one and add a commit description:

  • what the change is doing at least
  • the GNU ChangeLog part

Comment on lines 27 to 30
let _pop1 = ctpop(42i32);
let _pop2 = ctpop(42u32);
let _pop3 = ctpop(42i128);
let _pop4 = ctpop(42u128);
Copy link
Member

Choose a reason for hiding this comment

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

I think the test can be made to check the actual result. You could do something similar to the above, or you could even create a dedicated file that only checks ctpop.

Copy link
Author

Choose a reason for hiding this comment

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

I tried to put this in execute, but I encountered the following error:

/usr/bin/ld: /workspace/gccrs-build/demo.rs:11: undefined reference to `__builtin_popcountg'

Since this builtin was introduced in gcc 14, I am wondering if the related patch has been merged into the current gcc codebase?

Copy link
Member

Choose a reason for hiding this comment

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

"gcc 14" is a bit too vague, as we're really tracking gcc master branch. If the builtin is missing, then we can't really merge it now I guess.

Copy link
Author

Choose a reason for hiding this comment

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

I searched for this function in the repository, and it actually appears in the test case. So it's strange that we can't find the symbol. Am I doing something wrong, like missing a necessary runtime library that needs to be linked?

@c8ef c8ef requested a review from dkm November 11, 2024 01:30
@powerboat9
Copy link
Contributor

Try passing -fdump-tree-gimple to the compiler, and comparing the generated files with these


tree popcount_builtin = error_mark_node;

BuiltinsContext::get ().lookup_simple_builtin ("__builtin_popcountg",
Copy link
Member

Choose a reason for hiding this comment

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

why popcountg and not popcount here?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe check that the builtin is correctly registered and found (rust-builtins.cc in define_builtins() and lookup_gcc_builtin.

Copy link
Author

Choose a reason for hiding this comment

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

why popcountg and not popcount here?

Because it is a generic version, it can accommodate various integral types.

Copy link
Author

Choose a reason for hiding this comment

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

It seems to be working now - please take another look :)

@c8ef c8ef force-pushed the master branch 3 times, most recently from 42d7d27 to ec27138 Compare November 12, 2024 15:48
@c8ef c8ef requested a review from dkm November 12, 2024 16:24
@powerboat9
Copy link
Contributor

It looks like you're getting errors on the 32 bit testsuites and the gcc4.8 one

@c8ef
Copy link
Author

c8ef commented Nov 13, 2024

For the program below:

int main() { return __builtin_popcountg((unsigned __int128)45); }

The current version of gccrs will produce the following error when using -m32:

demo.c: In function ‘main’:
demo.c:1:51: error: ‘__int128’ is not supported on this target
    1 | int main() { return __builtin_popcountg((unsigned __int128)45); }
      |                                                   ^~~~~~~~

@dkm
Copy link
Member

dkm commented Nov 13, 2024

For the program below:

int main() { return __builtin_popcountg((unsigned __int128)45); }

The current version of gccrs will produce the following error when using -m32:

demo.c: In function ‘main’:
demo.c:1:51: error: ‘__int128’ is not supported on this target
    1 | int main() { return __builtin_popcountg((unsigned __int128)45); }
      |                                                   ^~~~~~~~

int128 are probably not supported for this target. You need to adjust your test. In C you'd test a macro to check support for int128, but not sure how that is supposed to work in rust.

@powerboat9
Copy link
Contributor

I think we should be checking int_n_enabled_p and calling rust_sorry_at if someone tries to use a u128 when gcc doesn't support it

@powerboat9
Copy link
Contributor

And then once libcore compiles, try to get i128/u128 working on more targets

@c8ef
Copy link
Author

c8ef commented Nov 13, 2024

I'm unsure if rustc supports 128-bit types on the x32 platform. Also, note that clang still cannot compile this on the armv7a target: https://godbolt.org/z/77q7Eee5M.

EDIT: Yes https://godbolt.org/z/459e3zssY.

@powerboat9
Copy link
Contributor

It's probably fine to just skip u128/i128 support for now -- "perfect is the enemy of good" and all

@dkm
Copy link
Member

dkm commented Nov 13, 2024

@c8ef why do you close your PR?

@c8ef
Copy link
Author

c8ef commented Nov 14, 2024

Sorry, I was trying to re-trigger the CI and got interrupted by other things.
Please take another look. @powerboat9 @dkm

@c8ef
Copy link
Author

c8ef commented Nov 14, 2024

Oops, there are still two i128 values in the test case.😵 I will further revise the test case tonight.

This patch adds support for the `ctpop` intrinsic.

gcc/rust/ChangeLog:

	* backend/rust-compile-intrinsic.cc (ctpop_hander): Add `ctpop`.

gcc/testsuite/ChangeLog:

	* rust/execute/torture/ctpop.rs: New test.

Signed-off-by: Yuao Ma <[email protected]>
@dkm
Copy link
Member

dkm commented Nov 14, 2024

You don't need to close the PR everytime there's something going on. You can mark it as draft to make it clear you're still working on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants