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.

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

Add -Z small-data-threshold #117465

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add -Z small-data-threshold #117465

wants to merge 1 commit into from
+467 −55

Conversation

paulmenage
Copy link
Contributor

@paulmenage paulmenage commented Oct 31, 2023 •
edited
Loading

This flag allows specifying the threshold size above which LLVM should not consider placing small objects in a .sdata or .sbss section.

Support is indicated in the target options via the small-data-threshold-support, which can (currently) be either via an LLVM argument or an LLVM module flag.

@rustbot
Copy link
Collaborator

rustbot commented Oct 31, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @b-naber (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 31, 2023
@paulmenage
Copy link
Contributor Author

paulmenage commented Oct 31, 2023 •
edited
Loading

This is a more targeted version of what I was trying to achieve with #116555 (which would add -Z llvm-module-flag to enable the same goal on RISCV).

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@paulmenage paulmenage force-pushed the small-data-limit branch 3 times, most recently from 9511b44 to f4f0492 Compare November 1, 2023 06:30
@b-naber
Copy link
Contributor

b-naber commented Nov 17, 2023

Sorry for the late reply. I think this needs an MCP before it can be merged.

@paulmenage paulmenage mentioned this pull request Nov 17, 2023
Closed
3 tasks
@bors
Copy link
Contributor

bors commented Nov 22, 2023

☔ The latest upstream changes (presumably #118178) made this pull request unmergeable. Please resolve the merge conflicts.

@b-naber
Copy link
Contributor

b-naber commented Dec 9, 2023

This could be merged now. Can you rebase?

@paulmenage
Copy link
Contributor Author

There's still some lack of clarity on the MCP zulip stream whether this is the right way to solve the problem, versus a target feature. I'll push the rebase but it's probably best to not merge it yet.

@paulmenage
Copy link
Contributor Author

OK, the conclusion on the zulip chat appeared to be that a target feature wasn't the right approach, so I think this is ready.

@apiraino
Copy link
Contributor

@b-naber can you r+? If I understand this patch is ripe for merging. thanks.

@compiler-errors
Copy link
Member

r? compiler-errors @bors r+

@bors
Copy link
Contributor

bors commented Feb 5, 2024

📌 Commit a618f74 has been approved by compiler-errors

It is now in the queue for this repository.

@compiler-errors
Copy link
Member

Funny because I was going to ask the same thing, but then I removed it. Given that someone else cares about the same thing, let's implement that warning then.

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 5, 2024
@paulmenage
Copy link
Contributor Author

Where would be the most appropriate place to put that check?

@paulmenage
Copy link
Contributor Author

Is there a way to issue a warning if a flag's value isn't used? Given the way LLVM supports small-data thresholds in two completely different ways (arg versus module flag) we'd be duplicating the arch checks if we put a warning at the top level. If we could check after the build that the flag had been used that would be less fragile.

@JohnCSimon
Copy link
Member

@paulmenage
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you are going to continue please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this May 26, 2024
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label May 26, 2024
@paulmenage
Copy link
Contributor Author

I have an updated version of this PR, but I don't see the "Reopen pull request" button (presumably because I'm not an admin on rust-lang/rust).

@bjorn3
Copy link
Member

bjorn3 commented Jul 4, 2024

You can only reopen if you force push the commit you had on this branch before closong the PR. This is a quirk of github.

@paulmenage
Copy link
Contributor Author

I haven't pushed a new version of the PR though, I just have it locally. The version on github is still the version from January. But I still don't have a "Reopen pull request" button on my view of the PR page. (I see that button on closed PRs on a repo for which I am an admin)

@compiler-errors
Copy link
Member

You can try to push now

@paulmenage
Copy link
Contributor Author

Thanks :-)

@rustbot
Copy link
Collaborator

rustbot commented Jul 4, 2024

These commits modify compiler targets.
(See the Target Tier Policy.)

@paulmenage
Copy link
Contributor Author

I'm definitely a little uncomfortable with the need to modify the numerous RiscV and MIPS targets to indicate the support for small-data-threshold although it's cleaner than the ad-hoc support I had in the previous version of the patch.

Is there a nice way to apply default target options for an architecture? (Currently the defaults all seem to be defined for an OS rather than for an arch.)

@paulmenage
Copy link
Contributor Author

Another option would be to have a DefaultForArch value of SmallDataThresholdSupport, which would be the default value in a target; we'd interpret DefaultForArch as appropriate on riscv/MIPS architectures, and as None elsewhere. Then there wouldn't be a need to modify most of the target files (nor existing .json target definitions)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 9, 2024
@oskgo oskgo removed the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Jul 11, 2024
@bors
Copy link
Contributor

bors commented Jul 22, 2024

☔ The latest upstream changes (presumably #128063) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot
Copy link
Collaborator

rustbot commented Aug 27, 2024

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

@paulmenage
Copy link
Contributor Author

Oops, some unintentional reformatting crept in there, I'll fix that.

@paulmenage
Add -Z small-data-threshold
06b8ea0 
This flag allows specifying the threshold size above which LLVM should
not consider placing small objects in a .sdata or .sbss section.

Support is indicated in the target options via the
small-data-threshold-support, which can be either via an LLVM argument
or an LLVM module flag.
@bors
Copy link
Contributor

bors commented Aug 29, 2024

☔ The latest upstream changes (presumably #129750) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compiler-errors compiler-errors Awaiting requested review from compiler-errors

Assignees

@compiler-errors compiler-errors

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet