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

Set metadata for vtable-related loads #39995

Merged
merged 2 commits into from
Feb 25, 2017
Merged

Conversation

Aatch
Copy link
Contributor

@Aatch Aatch commented Feb 21, 2017

Give LLVM much more information about vtable pointers. Without the extra
information, LLVM has to be rather pessimistic about vtables, preventing
a number of obvious optimisations.

  • Makes the vtable pointer argument noalias and readonly.
  • Marks loads of the vtable pointer as nonnull.
  • Marks load from the vtable with !invariant.load metadata.

Fixes #39992

Give LLVM much more information about vtable pointers. Without the extra
information, LLVM has to be rather pessimistic about vtables, preventing
a number of obvious optimisations.

* Makes the vtable pointer argument noalias and readonly.
* Marks loads of the vtable pointer as nonnull.
* Marks load from the vtable with `!invariant.load` metadata.

Fixes rust-lang#39992
@rust-highfive
Copy link
Collaborator

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@Aatch
Copy link
Contributor Author

Aatch commented Feb 21, 2017

LLVM IR before and after this patch: https://gist.github.com/Aatch/fe555f251de910d7e765a11cbb5d78d4

@arielb1
Copy link
Contributor

arielb1 commented Feb 21, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Feb 21, 2017

📌 Commit 7af3406 has been approved by arielb1

@bors
Copy link
Contributor

bors commented Feb 21, 2017

⌛ Testing commit 7af3406 with merge 9436c01...

@bors
Copy link
Contributor

bors commented Feb 21, 2017

💔 Test failed - status-appveyor

@arielb1
Copy link
Contributor

arielb1 commented Feb 21, 2017

unchanged test:

C:\projects\rust\src/test\codegen\function-arguments.rs:124:11: error: expected string not found in input
// CHECK: @trait_borrow(i8* nonnull, void (i8*)** nonnull)
          ^
C:\projects\rust\build\x86_64-pc-windows-gnu\test\codegen\function-arguments.ll:266:62: note: scanning from here
define internal void @str(i8* noalias nonnull readonly, i64) unnamed_addr #1 {
                                                             ^
C:\projects\rust\build\x86_64-pc-windows-gnu\test\codegen\function-arguments.ll:276:22: note: possible intended match here
define internal void @trait_borrow(i8* nonnull, void (i8*)** noalias nonnull readonly) unnamed_addr #1 {

@Aatch
Copy link
Contributor Author

Aatch commented Feb 21, 2017

@bors r=arielb1

@bors
Copy link
Contributor

bors commented Feb 21, 2017

📌 Commit d80cf80 has been approved by arielb1

@bluss bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Feb 22, 2017
eddyb added a commit to eddyb/rust that referenced this pull request Feb 25, 2017
Set metadata for vtable-related loads

Give LLVM much more information about vtable pointers. Without the extra
information, LLVM has to be rather pessimistic about vtables, preventing
a number of obvious optimisations.

* Makes the vtable pointer argument noalias and readonly.
* Marks loads of the vtable pointer as nonnull.
* Marks load from the vtable with `!invariant.load` metadata.

Fixes rust-lang#39992
eddyb added a commit to eddyb/rust that referenced this pull request Feb 25, 2017
Set metadata for vtable-related loads

Give LLVM much more information about vtable pointers. Without the extra
information, LLVM has to be rather pessimistic about vtables, preventing
a number of obvious optimisations.

* Makes the vtable pointer argument noalias and readonly.
* Marks loads of the vtable pointer as nonnull.
* Marks load from the vtable with `!invariant.load` metadata.

Fixes rust-lang#39992
bors added a commit that referenced this pull request Feb 25, 2017
@bors bors merged commit d80cf80 into rust-lang:master Feb 25, 2017
@gnzlbg
Copy link
Contributor

gnzlbg commented Mar 1, 2017

@Aatch do you have a benchmark/examples for this somewhere? Like to be able to see the "before" and "after" on the generated IR? LLVM devirtualization has improved a lot in the last year (still pretty basic though), but it would be nice to have a couple of "benchmarks" for cases in which we expect a backend to be able to devirtualize calls. That might also help filling LLVM bugs for the cases in which this is not the case, or discover situations in which we are not passing meta-data correctly.

let meta = if meta_ty.element_type().kind() == llvm::TypeKind::Pointer {
b.load_nonnull(meta, None)
} else {
b.load(meta, None)
Copy link
Member

Choose a reason for hiding this comment

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

Missing alignment?

Copy link
Contributor

Choose a reason for hiding this comment

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

None = ABI alignment. Vtables are ABI aligned.

info.attrs.set(ArgAttribute::NonNull);
info.attrs.set(ArgAttribute::ReadOnly);
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't readonly a function attribute? What does this mean when applied to arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When applied to a pointer argument, it means that the function will only read from it. That allows LLVM to assume that the value behind the pointer won't change across a call to that function.

@dpc
Copy link
Contributor

dpc commented Mar 4, 2017

Please take a look at my last comment in #39992 about tight loops and this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants