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

[MIR] non-zeroing drop #33622

Merged
merged 11 commits into from
Jun 5, 2016
Merged

[MIR] non-zeroing drop #33622

merged 11 commits into from
Jun 5, 2016

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented May 13, 2016

This enables non-zeroing drop through stack flags for MIR.

Fixes #30380.
Fixes #5016.

@rust-highfive
Copy link
Collaborator

r? @nrc

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

@arielb1
Copy link
Contributor Author

arielb1 commented May 13, 2016

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned nrc May 13, 2016
@solson
Copy link
Member

solson commented May 13, 2016

After this MIR pass, will all MIR drops be unconditional? (That is, any conditional drops will be guarded by explicit conditional branching in the MIR.)

@arielb1
Copy link
Contributor Author

arielb1 commented May 13, 2016

@solson

Right.

@arielb1 arielb1 changed the title [WIP] [MIR] Non-zeroing drop [MIR] non-zeroing drop May 14, 2016
@arielb1
Copy link
Contributor Author

arielb1 commented May 14, 2016

Passes make check. I should do something about @pnkfelix's warning-ridden code, but otherwise it's ready.

@durka
Copy link
Contributor

durka commented May 14, 2016

cc #5016

This is awesome! Do we have benchmarks? Will functions like std::ptr::read_and_drop go away or can they be refitted to set the drop flag instead of filling?

@arielb1
Copy link
Contributor Author

arielb1 commented May 14, 2016

Performance numbers:

stage1 MIR librustc (old):

21:26:53 O: time: 17.426; rss: 1510MB   translation
21:26:53 O: time: 0.003; rss: 1510MB    assert dep graph
21:26:53 O: time: 0.000; rss: 1510MB    serialize dep graph
21:27:00 O:   time: 6.139; rss: 922MB   llvm function passes [0]
21:29:08 O:   time: 128.802; rss: 1004MB        llvm module passes [0]
21:29:45 O:   time: 35.232; rss: 1009MB codegen passes [0]
21:29:46 O:   time: 0.010; rss: 228MB   codegen passes [0]
21:29:46 O: time: 172.367; rss: 212MB   LLVM passes

stage2 MIR librustc:

21:46:37 O: time: 25.428; rss: 1319MB   translation
21:46:37 O: time: 0.003; rss: 1319MB    assert dep graph
21:46:37 O: time: 0.000; rss: 1319MB    serialize dep graph
21:46:43 O:   time: 5.261; rss: 1131MB  llvm function passes [0]
21:48:16 O:   time: 92.960; rss: 1133MB llvm module passes [0]
21:48:46 O:   time: 29.626; rss: 1137MB codegen passes [0]
21:48:47 O:   time: 0.005; rss: 1070MB  codegen passes [0]
21:48:47 O: time: 130.045; rss: 1051MB  LLVM passes

@arielb1
Copy link
Contributor Author

arielb1 commented May 14, 2016

stage 3 MIR librustc (there was some perf issue with stage2 apparently).

22:19:57 O: time: 17.890; rss: 1263MB   translation
22:19:57 O: time: 0.003; rss: 1263MB    assert dep graph
22:19:57 O: time: 0.000; rss: 1263MB    serialize dep graph
22:20:02 O:   time: 4.530; rss: 782MB   llvm function passes [0]
22:21:25 O:   time: 82.637; rss: 847MB  llvm module passes [0]
22:21:49 O:   time: 22.951; rss: 862MB  codegen passes [0]
22:21:49 O:   time: 0.008; rss: 221MB   codegen passes [0]
22:21:49 O: time: 111.719; rss: 221MB   LLVM passes

Non -Z orbit:

time: 18.584; rss: 1219MB       translation
time: 0.003; rss: 1219MB        assert dep graph
time: 0.000; rss: 1219MB        serialize dep graph
  time: 3.910; rss: 711MB       llvm function passes [0]
  time: 89.009; rss: 814MB      llvm module passes [0]
  time: 23.172; rss: 811MB      codegen passes [0]
  time: 0.008; rss: 221MB       codegen passes [0]
time: 117.952; rss: 220MB       LLVM passes

@bors
Copy link
Contributor

bors commented May 14, 2016

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

@arielb1
Copy link
Contributor Author

arielb1 commented May 14, 2016

perf after rebase:

16:14:04 O: time: 17.806; rss: 1254MB   translation
16:14:04 O: time: 0.003; rss: 1254MB    assert dep graph
16:14:04 O: time: 0.000; rss: 1254MB    serialize dep graph
16:14:08 O:   time: 4.457; rss: 783MB   llvm function passes [0]
16:15:32 O:   time: 83.976; rss: 858MB  llvm module passes [0]
16:15:55 O:   time: 22.197; rss: 854MB  codegen passes [0]
16:15:56 O:   time: 0.007; rss: 229MB   codegen passes [0]
16:15:56 O: time: 112.204; rss: 213MB   LLVM passes
16:15:56 O:   time: 0.340; rss: 213MB   running linker
16:15:57 O: time: 1.180; rss: 213MB     linking

I get the feeling that SimpifyCfg is not doing its work.

@dotdash
Copy link
Contributor

dotdash commented May 15, 2016

@arielb1 What makes you think that? IOW, what do you expect it to do that doesn't happen?

@arielb1
Copy link
Contributor Author

arielb1 commented May 15, 2016

Just that there are no perf improvements over its merge. Maybe LLVM just does this by itself.

@pnkfelix
Copy link
Member

@nikomatsakis @arielb1 I was under the impression that I would be given a chance to put up a properly revised PR of the data flow fixes (cleaning up the warnings and the commit history).

I obviously can't stop people from merging this PR as is, but it would be a little irritating to me if that happened

@pnkfelix
Copy link
Member

(In any case, I'll try to soon put up a separate PR with just the data flow fixes, that will include the relevant changes contributed by @arielb1 from this PR, and link to this PR when i do. hopefully that will happen in the next eight hours. I recommend waiting until that happens before initiating formal code review, at least if you plan to go commit by commit)

@arielb1
Copy link
Contributor Author

arielb1 commented May 16, 2016

@pnkfelix

I just wanted to open my code for review. Will rebase on top of yours.

@dotdash
Copy link
Contributor

dotdash commented May 16, 2016

@arielb1 Yeah, LLVM does this just fine, the point of having it as a MIR pass is that we a) generate a little less bad code for the unoptimized case, and b) can possibly do more optimizations that are local to a single basic block instead of having to look across block boundaries.

/// Drop the Lvalue and write an operand over it
DropAndReplace {
location: Lvalue<'tcx>,
value: Operand<'tcx>,
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, using value and replacement might be a better idea here. Having value mean different things for Drop and DropAndReplace seems confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll just use location for both.

@arielb1
Copy link
Contributor Author

arielb1 commented May 16, 2016

@dotdash

c) humans can actually read the generated MIR.

That's 3 good reasons.

I might just add a span for temporaries. For debugging and readability, you know.

@pnkfelix
Copy link
Member

@arielb1

okay, thanks. (Sorry for any cranky old man attitude in my response; it was 5 in the morning and I (unlike niko) don't operate well at that hour at this point in my life.)

I have posted the dataflow fixes PR at #33667 ; I tried to extract the relevant portions of the things you fixes in the dataflow into that PR, while leaving out the stuff that was solely for your non-zeroing drop work. So while it probably won't be a 100% trivial rebase, it shouldn't be too onerous, I hope.

@arielb1
Copy link
Contributor Author

arielb1 commented May 17, 2016

rebased on top of #33667

DropAndReplace { target, unwind: Some(unwind), .. } |
Drop { target, unwind: Some(unwind), .. } =>
vec![target, unwind].into_cow(),
DropAndReplace { ref target, .. } | Drop { ref target, .. } =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: can you write unwind: None here to make this more robust against accidental re-ordering? (and more obvious, I missed the lines above for a while)

@arielb1
Copy link
Contributor Author

arielb1 commented May 18, 2016

fixed nit

@bors
Copy link
Contributor

bors commented Jun 4, 2016

💔 Test failed - auto-win-msvc-64-opt-mir

@arielb1
Copy link
Contributor Author

arielb1 commented Jun 4, 2016

@arielb1
Copy link
Contributor Author

arielb1 commented Jun 4, 2016

Blocked on LLVM bug https://llvm.org/bugs/show_bug.cgi?id=28005

@eddyb
Copy link
Member

eddyb commented Jun 4, 2016

@arielb1 I'm not sure we can afford to wait for that to get fixed. Any chance we can work around it?

@nagisa
Copy link
Member

nagisa commented Jun 4, 2016

I’m having a hard time reproducing the issue in the gist it with the Arch’s 3.8 build of LLVM, which suggests that perhaps our own fork of LLVM went wrong somewhere?

@alexcrichton
Copy link
Member

Claimed to be fixed in llvm-mirror/llvm@8c4b617, I've backported it to our branch (rust-lang/llvm@80ad955), so @arielb1 wanna try updating and see if that fixes it?

@arielb1
Copy link
Contributor Author

arielb1 commented Jun 5, 2016

@alexcrichton

I manually-applied the fix and am testing whether it works. That error is fixed, but there is a different error from a different pass (now I make it to the codegen passes).

the *only* place where critical edges need to be broken is on Call
instructions, so only break them there.
@alexcrichton
Copy link
Member

Ok, just lemme know if anything needs backporting! These MSVC bugs have really bitten me in the past in terms of taking quite a long time to track down, but once you have a minimal IR reproduction it's generally ready to get reported to LLVM.

@arielb1
Copy link
Contributor Author

arielb1 commented Jun 5, 2016

rustc: /home/ariel/Rust/rust/src/llvm/include/llvm/ADT/DenseMap.h:1025: llvm::DenseMapIterator<KeyT, ValueT, KeyInfoT, Bucket, IsConst>::value_type* llvm::DenseMapIterator<KeyT, ValueT, KeyInfoT, Bucket, IsConst>::operator->() const [with KeyT = const llvm::MachineBasicBlock*; ValueT = int; KeyInfoT = llvm::DenseMapInfo<const llvm::MachineBasicBlock*>; Bucket = llvm::detail::DenseMapPair<const llvm::MachineBasicBlock*, int>; bool IsConst = false; llvm::DenseMapIterator<KeyT, ValueT, KeyInfoT, Bucket, IsConst>::pointer = llvm::detail::DenseMapPair<const llvm::MachineBasicBlock*, int>*; llvm::DenseMapIterator<KeyT, ValueT, KeyInfoT, Bucket, IsConst>::value_type = llvm::detail::DenseMapPair<const llvm::MachineBasicBlock*, int>]: Assertion `isHandleInSync() && "invalid iterator access!"' failed.

#0  0x00007f31bedbe478 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:55
#1  0x00007f31bedbf8fa in __GI_abort () at abort.c:89
#2  0x00007f31bedb73a7 in __assert_fail_base (fmt=<optimized out>, 
    assertion=assertion@entry=0x7f31b8ca2fe0 "isHandleInSync() && \"invalid iterator access!\"", 
    file=file@entry=0x7f31b8ca2dc8 "/home/ariel/Rust/rust/src/llvm/include/llvm/ADT/DenseMap.h", line=line@entry=1025, 
    function=function@entry=0x7f31b9374540 <llvm::DenseMapIterator<llvm::MachineBasicBlock const*, int, llvm::DenseMapInfo<llvm::MachineBasicBlock const*>, llvm::detail::DenseMapPair<llvm::MachineBasicBlock const*, int>, false>::operator->() const::__PRETTY_FUNCTION__> "llvm::DenseMapIterator<KeyT, ValueT, KeyInfoT, Bucket, IsConst>::value_type* llvm::DenseMapIterator<KeyT, ValueT, KeyInfoT, Bucket, IsConst>::operator->() const [with KeyT = const llvm::MachineBasicBl"...) at assert.c:92
#3  0x00007f31bedb7452 in __GI___assert_fail (assertion=0x7f31b8ca2fe0 "isHandleInSync() && \"invalid iterator access!\"", 
    file=0x7f31b8ca2dc8 "/home/ariel/Rust/rust/src/llvm/include/llvm/ADT/DenseMap.h", line=1025, 
    function=0x7f31b9374540 <llvm::DenseMapIterator<llvm::MachineBasicBlock const*, int, llvm::DenseMapInfo<llvm::MachineBasicBlock const*>, llvm::detail::DenseMapPair<llvm::MachineBasicBlock const*, int>, false>::operator->() const::__PRETTY_FUNCTION__> "llvm::DenseMapIterator<KeyT, ValueT, KeyInfoT, Bucket, IsConst>::value_type* llvm::DenseMapIterator<KeyT, ValueT, KeyInfoT, Bucket, IsConst>::operator->() const [with KeyT = const llvm::MachineBasicBl"...) at assert.c:101
#4  0x00007f31b8251fdf in llvm::BranchFolder::SplitMBBAt(llvm::MachineBasicBlock&, llvm::MachineBasicBlock::bundle_iterator<llvm::MachineInstr, llvm::ilist_iterator<llvm::MachineInstr> >, llvm::BasicBlock const*) ()
   from /home/ariel/Rust/rust/build-debug-assertions/x86_64-unknown-linux-gnu/stage1/bin/../lib/librustc_llvm-fe3cdf61.so
#5  0x00007f31b825274d in llvm::BranchFolder::CreateCommonTailOnlyBlock(llvm::MachineBasicBlock*&, llvm::MachineBasicBlock*, unsigned int, unsigned int&) () from /home/ariel/Rust/rust/build-debug-assertions/x86_64-unknown-linux-gnu/stage1/bin/../lib/librustc_llvm-fe3cdf61.so
#6  0x00007f31b8252c30 in llvm::BranchFolder::TryTailMergeBlocks(llvm::MachineBasicBlock*, llvm::MachineBasicBlock*) ()
   from /home/ariel/Rust/rust/build-debug-assertions/x86_64-unknown-linux-gnu/stage1/bin/../lib/librustc_llvm-fe3cdf61.so
#7  0x00007f31b8253cab in llvm::BranchFolder::TailMergeBlocks(llvm::MachineFunction&) [clone .part.237] ()
   from /home/ariel/Rust/rust/build-debug-assertions/x86_64-unknown-linux-gnu/stage1/bin/../lib/librustc_llvm-fe3cdf61.so
#8  0x00007f31b825481c in llvm::BranchFolder::OptimizeFunction(llvm::MachineFunction&, llvm::TargetInstrInfo const*, llvm::TargetRegisterInfo const*, llvm::MachineModuleInfo*) ()
   from /home/ariel/Rust/rust/build-debug-assertions/x86_64-unknown-linux-gnu/stage1/bin/../lib/librustc_llvm-fe3cdf61.so
#9  0x00007f31b8254bfa in (anonymous namespace)::BranchFolderPass::runOnMachineFunction(llvm::MachineFunction&) ()
   from /home/ariel/Rust/rust/build-debug-assertions/x86_64-unknown-linux-gnu/stage1/bin/../lib/librustc_llvm-fe3cdf61.so
#10 0x00007f31b8bb80f3 in llvm::FPPassManager::runOnFunction(llvm::Function&) ()
   from /home/ariel/Rust/rust/build-debug-assertions/x86_64-unknown-linux-gnu/stage1/bin/../lib/librustc_llvm-fe3cdf61.so
#11 0x00007f31b8bb84fb in llvm::FPPassManager::runOnModule(llvm::Module&) ()
   from /home/ariel/Rust/rust/build-debug-assertions/x86_64-unknown-linux-gnu/stage1/bin/../lib/librustc_llvm-fe3cdf61.so
#12 0x00007f31b8bb7c3d in llvm::legacy::PassManagerImpl::run(llvm::Module&) ()

Calling opt directly works fine, however.

@alexcrichton
Copy link
Member

Oh that's actually a red herring, it'll pass fine on the Windows bots. If you cross from Linux to Windows, you'll get that error, though. That's fixed in upstream LLVM we just haven't backported the fix yet.

Picks up the fix for PR28005
@arielb1
Copy link
Contributor Author

arielb1 commented Jun 5, 2016

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Jun 5, 2016

📌 Commit 063f882 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Jun 5, 2016

⌛ Testing commit 063f882 with merge f97c411...

bors added a commit that referenced this pull request Jun 5, 2016
[MIR] non-zeroing drop

This enables non-zeroing drop through stack flags for MIR.

Fixes #30380.
Fixes #5016.
@bors
Copy link
Contributor

bors commented Jun 5, 2016

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-32cross-opt, auto-linux-64-cargotest, auto-linux-64-cross-armhf, auto-linux-64-cross-armsf, auto-linux-64-cross-freebsd, auto-linux-64-cross-netbsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-cross-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild

@bors bors merged commit 063f882 into rust-lang:master Jun 5, 2016
bors added a commit that referenced this pull request Aug 1, 2016
Switch to MIR-based translation by default.

This patch makes `-Z orbit` default to "on", which means that by default, functions will be translated from Rust to LLVM IR through the upcoming MIR backend, instead of the antiquated AST backend.

This switch is made possible by the recently merged #33622, #33905 and smaller fixes.

If you experience any issues, please file a report for each of them. You can switch to the old backend to work around problems by either setting `RUSTFLAGS="-Zorbit=off"` or by annotating specific functions with `#[rustc_no_mir]` (which requires `#![feature(rustc_attrs)]` at the crate-level).

I would like this PR to get into nightly soon so that we can get early feedback in this release cycle and focus on correctness fixes and performance improvements, with the potential for removing the old backend implementation before beta branches off.

cc @rust-lang/compiler
bors added a commit that referenced this pull request Aug 2, 2016
Switch to MIR-based translation by default.

This patch makes `-Z orbit` default to "on", which means that by default, functions will be translated from Rust to LLVM IR through the upcoming MIR backend, instead of the antiquated AST backend.

This switch is made possible by the recently merged #33622, #33905 and smaller fixes.

If you experience any issues, please file a report for each of them. You can switch to the old backend to work around problems by either setting `RUSTFLAGS="-Zorbit=off"` or by annotating specific functions with `#[rustc_no_mir]` (which requires `#![feature(rustc_attrs)]` at the crate-level).

I would like this PR to get into nightly soon so that we can get early feedback in this release cycle and focus on correctness fixes and performance improvements, with the potential for removing the old backend implementation before beta branches off.

cc @rust-lang/compiler
@brson brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Aug 2, 2016
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.