Skip to content

WIP/ POC Add no_iu128_fmt config to disable 128 bit integer formatting. #136385

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

Closed

Conversation

onestacked
Copy link
Contributor

@onestacked onestacked commented Feb 1, 2025

Add a new config option no_iu128_fmt to core, which disables some u/i 128 formatting in order to reduce the need for some division intrinsics in the linux kernel.

RFC patch for linux that uses this:
https://lore.kernel.org/rust-for-linux/[email protected]/T/#u

@rustbot
Copy link
Collaborator

rustbot commented Feb 1, 2025

r? @thomcc

rustbot has assigned @thomcc.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 1, 2025
@onestacked

This comment was marked as outdated.

@rustbot
Copy link
Collaborator

rustbot commented Feb 1, 2025

Failed to set assignee to ghost: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

@rust-log-analyzer

This comment has been minimized.

@@ -638,21 +639,29 @@ fn parse_u64_into<const N: usize>(mut n: u64, buf: &mut [MaybeUninit<u8>; N], cu
#[stable(feature = "rust1", since = "1.0.0")]
impl fmt::Display for u128 {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
fmt_u128(*self, true, f)
if cfg!(no_iu128_fmt) {
panic!("u128 formatting support is turned off")
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to disable the Display impl entirely instead?

Copy link
Contributor Author

@onestacked onestacked Feb 1, 2025

Choose a reason for hiding this comment

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

I've tried a bit but removing it entirely would be a lot more intrusive since it is used in some other parts of core.

I might be able to remove it, the current implementation is mostly a POC for a quick test.

Doing so would also need to disable NonZeroI/U128 Debug, which doesn't seem easy.

@onestacked onestacked changed the title Add no_iu128_fmt config to disable 128 bin integer formatting. Add no_iu128_fmt config to disable 128 bit integer formatting. Feb 1, 2025
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this pull request Feb 1, 2025
This removes the panicking implementation of the x86 intrinsics.

This requires a PR [0] for the rust core library to remove the need for
the `__udivti3` intrinsic. This PR disables formatting support for the
`i/u128` rust primitives when the `no_iu128_fmt` config option is set.

In order to remove the need for the other intrinsics a recent rust
version is required (Tested with 1.84).

Link: rust-lang/rust#136385 [0]
Signed-off-by: Christian Schrefl <[email protected]>
@workingjubilee
Copy link
Member

The issue at hand is not the formatting implementation, it is the division code, correct? This should be captured directly in the PR description because it is the actual direct motivation and the PR description usually includes the motivation.

What platforms actually require __udivti3?

@onestacked
Copy link
Contributor Author

It's needed at least on x86_64

@onestacked onestacked changed the title Add no_iu128_fmt config to disable 128 bit integer formatting. WIP/ POC Add no_iu128_fmt config to disable 128 bit integer formatting. Feb 1, 2025
@workingjubilee
Copy link
Member

Yeah, it seems everywhere needs it, on closer review. The gcc spec for what it does is confusing, but it's the 128-bit integer division intrinsic.

@onestacked
Copy link
Contributor Author

It seems this was already attempted, but closed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants