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

Consider aggressively avoiding generating memcpys for moves if we can avoid it #13707

Open
pcwalton opened this issue Apr 23, 2014 · 8 comments
Labels
A-codegen Area: Code generation A-mir-opt Area: MIR optimizations A-mir-opt-nrvo Fixed by NRVO C-enhancement Category: An issue proposing an enhancement or a PR with one. I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@pcwalton
Copy link
Contributor

Given that our assembler output tends to have a lot of moves in it and LLVM is bad at optimizing them out, I'm beginning to wonder if we shouldn't take more aggressive steps. For example:

  • Convert (non-POD?) by-value move parameters in the Rust ABI to be passed by reference.
  • Non-immediate values that go dead by dint of being moved into a new place should not be memcpy'd there.

Any others?

@thestinger
Copy link
Contributor

Is there any reason to use memcpy instead of a load instruction when we're only copying a single element with a known type? It seems like the reasons for doing it may no longer be true. If the issue is that it generates poor code with large types and --opt-level=0 we could teach LLVM to output a memcpy for those loads (if it doesn't already).

@steveklabnik
Copy link
Member

Triage: no change

@steveklabnik
Copy link
Member

Triage: I know @pcwalton was doing some work on a related area recently, but don't believe it bore any fruit.

@pcwalton
Copy link
Contributor Author

I'm going to resurrect this bug as a tracking bug for various bad memcpy instances.

Here's one that has been affecting Stylo (cc @bholley): https://bugzilla.mozilla.org/show_bug.cgi?id=1380198

@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 21, 2017
@nox
Copy link
Contributor

nox commented Mar 31, 2018

Let's close this or actually treat it as a tracking bug and put more details in it, it's not currently very actionable. I vote for closing it. Cc @rust-lang/compiler @rust-lang/wg-codegen

@oli-obk
Copy link
Contributor

oli-obk commented Mar 31, 2018

I think we can address a few of these issues with mir optimizations and ABI adjustments as noted above.

  • ABI: large by move values are actually just passed as pointers to the memory, expose this in MIR so we can do fancy stuff like deduplicating large in let large = foo(large); if foo does return the argument again (after modifications).
  • dedup locals (in MIR, not sure how good of an idea that is)
    let foo = large; drop(foo); let bar = other_large; (foo and bar should have the same local)

@eddyb
Copy link
Member

eddyb commented Apr 1, 2018

The plan is getting something like #47954 into the compiler in the coming months.

@eddyb
Copy link
Member

eddyb commented Apr 1, 2018

@oli-obk your last point can be achieved by moving the StorageDead earlier (shrinking "storage liveness ranges"), which will result in LLVM reusing the stack space.

@jonas-schievink jonas-schievink added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-mir-opt Area: MIR optimizations labels Apr 18, 2020
@jonas-schievink jonas-schievink added the A-mir-opt-nrvo Fixed by NRVO label May 14, 2020
arcnmx pushed a commit to arcnmx/rust that referenced this issue Dec 17, 2022
Add `move_const_to_impl` assist

Closes rust-lang#13277

For the initial implementation, this assist:
- only applies to inherent impl. Much as we can *technically* provide this assist for default impl in trait definitions, it'd be complicated to get it right.
- may break code when the const's name collides with an item of a trait the self type implements.

Comments in the code explain those caveats in a bit more detail.
flip1995 pushed a commit to flip1995/rust that referenced this issue Dec 15, 2024
)

rust-lang#133150

This is more likely to be intended as an intra-doc link than it is to be
intended as a refdef. If a refdef is intended, it does not need to be
nested within a list item.

```markdown
- [`LONG_INTRA_DOC_LINK`]: this
  looks like an intra-doc link,
  but is actually a refdef.
  The first line will seem to
  disappear when rendered as HTML.
```

> - [`LONG_INTRA_DOC_LINK`]: this
>   looks like an intra-doc link,
>   but is actually a refdef.
>   The first line will seem to
>   disappear when rendered as HTML.

changelog: [`doc_nested_refdefs`]: add suspicious lint for link def at
start of list items and block quotes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-mir-opt Area: MIR optimizations A-mir-opt-nrvo Fixed by NRVO C-enhancement Category: An issue proposing an enhancement or a PR with one. I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants