Translate array drop glue using MIR #41917

arielb1 · 2017-05-11T19:11:01Z

I was a bit lazy here and used a usize-based index instead of a pointer iteration. Do you think this is important @eddyb?

r? @eddyb

eddyb · 2017-05-11T19:17:49Z

Can you check the optimized IR? Ideally we'd even have a test - I believe LLVM will turn it into a pointer loop.

jonas-schievink · 2017-05-11T19:18:41Z

src/librustc/ty/util.rs

@@ -578,6 +578,15 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
            bug!("empty_substs_for_def_id: {:?} has type parameters", item_def_id)
        })
    }
+
+    pub fn const_usize(&self, val: usize) -> ConstInt {


Shouldn't this usize be a u64 to be correct when compiling from 32 bit to 64?

In practice val is either 0 or 1.

I see, it's probably ok then (still, it'd be nice to prevent misuses of this function)

u16 is probably a good option for the type then, so one can never pass a value too large.

nagisa

A few nits. I only skimmed translator changes, nothing obviously wrong there.

nagisa · 2017-05-11T23:39:28Z

src/librustc_mir/util/elaborate_drops.rs

+            is_cleanup,
+            terminator: Some(Terminator {
+                source_info: self.source_info,
+                kind: TerminatorKind::Resume,


NIT: Unreachable here would make it much more obvious this terminator is patched out a bit later.

nagisa · 2017-05-11T23:41:01Z

src/librustc_mir/util/elaborate_drops.rs

+    ///    can_go = index < len
+    ///    if can_go then drop-block else succ
+    /// drop-block:
+    ///    ptr = &mut LV[len]


NIT: s/len/index/?

nagisa · 2017-05-11T23:46:07Z

src/librustc_mir/util/elaborate_drops.rs

+
+        let is_cleanup = self.is_cleanup;
+        let succ = self.succ; // FIXME(#6393)
+        let loop_block = self.drop_loop(unwind, succ, index, length, ety, is_cleanup);


Is it guaranteed that is_cleanup is always false here? IIRC all the details related to this, it is guaranteed, in which this is_cleanup flag is redundant (unwind being None or Some already carries that information).

is_cleanup is technically always false here because this can't be reached from "normal" drop elaboration, but if we fixed drop glue for arrays it would be true.

nagisa · 2017-05-11T23:49:37Z

src/librustc_mir/util/elaborate_drops.rs

@@ -691,4 +808,12 @@ impl<'l, 'b, 'tcx, D> DropCtxt<'l, 'b, 'tcx, D>
        let mir = self.elaborator.mir();
        self.elaborator.patch().terminator_loc(mir, bb)
    }
+
+    fn constant_usize(&self, val: usize) -> Operand<'tcx> {


Ditto here wrt usize/u16.

eddyb · 2017-05-12T05:38:41Z

src/librustc_mir/util/elaborate_drops.rs

+
+        let is_cleanup = self.is_cleanup;
+        let succ = self.succ; // FIXME(#6393)
+        let loop_block = self.drop_loop(unwind, succ, index, length, ety, is_cleanup);


I'm worried about this code repeating itself for all used array lengths - would it be possible to have the array drop coerce to a *[T] and drop that? Or is thst premature optimization?

The code in question is fairly compact. I feel like LLVM would inline it in all cases anyway. The only really troublesome case I can imagine is somebody having a very large number of small arrays on stack.

I was more worried about us instantiating so many copies of pretty much identical IR. But thinking more about it, various lengths of fixed-length arrays are somewhat rare in practice, so this might not be a problem at all.

I expect LLVM to always inline this anyway. Note that this is translated inline today.

arielb1 · 2017-05-12T13:13:23Z

Can you check the optimized IR? Ideally we'd even have a test - I believe LLVM will turn it into a pointer loop.

Nope it doesn't. I'll try to write up pointer loops tomorrow.

Mark-Simulacrum · 2017-05-15T12:31:23Z

src/librustc_mir/util/elaborate_drops.rs

@@ -323,7 +316,8 @@ impl<'l, 'b, 'tcx, D> DropCtxt<'l, 'b, 'tcx, D>
             self.elaborator.field_subpath(self.path, Field::new(i)))
        }).collect();

-        self.drop_ladder(fields).0
+        let (succ, unwind) = (self.succ, self.unwind); // FIXME(#6393)


The issue noted in the FIXME (#6393) is closed... should it be changed to a different issue? This is also the case in a few other places in the code.

You can still easily track where to look from the comments in the issue (i.e. the niko’s in favour comment).

EDIT: nice trick. Amusing.

nagisa · 2017-05-15T13:09:58Z

There’s some travis failures due to MIR variants being boxed now, @arielb1. You might want to rebase.

nagisa · 2017-05-15T13:12:09Z

src/librustc_mir/util/elaborate_drops.rs

-            succ = self.drop_subpath(lv, path, succ, unwind_succ);
-            succ
-        }).collect()
+        Some(succ).into_iter().chain(


nit: iter::once(succ) is a more compact way to express this.

nagisa · 2017-05-15T13:28:41Z

The last commit looks good.

alexcrichton · 2017-05-18T20:20:46Z

@arielb1 is this waiting on review now? I couldn't immediately tell from the state of the PR. Just curious for what tags too apply!

arielb1 · 2017-05-18T20:23:36Z

@alexcrichton

I'm trying to see whether I can get pointer-based iteration to work out nicely enough I'll want to use it.

nagisa · 2017-05-20T11:37:58Z

I feel like code could be a bit more clear if the index-based loop and the pointer-based loop were separate, rather than so closely intermingled in the same function.

It is also not entirely clear to me that both BinOp::Offset and NullaryOp::SizeOf are necessary. That is, the Offset could be just a Add(x, SizeOf::<T>()).

These are the nits, feel free to ignore them.

Now… I think it could be possible to avoid indexing the array altogether in the loop. That is, instead of generating:

    /// loop-block:
    ///    can_go = index_or_cur == length_or_end
    ///    if can_go then succ else drop-block
    /// drop-block:
    ///    if size_of::<T> != 0 {
    ///        ptr = index_or_cur
    ///        index_or_cur = index_or_cur + size_of::<T>()
    ///    } else {
    ///        ptr = &mut LV[index_or_cur]
    ///        index_or_cur = index_or_cur + 1
    ///    }
    ///    drop(ptr);

It could be simplified(?) to this instead:

drop_selector:
    ptr = &mut LV[0];
    index = 0;
    len = len(LV);
    is_zero_sized = size_of::<T>() == 0;
    if is_zero_sized then zs_head else loop_head;
zs_head:
    finished = index == len;
    if finished then succ else zs_body;
zs_body:
    drop(ptr);
    index = index + 1;
    goto zs_head;
loop_head:
    finished = ptr == end;
    if finished then succ else loop_body
loop_body:
    drop(ptr);
    ptr = ptr + size_of::<T>();
    goto loop_head;

note the lack of actual “indexing” in the zero-sized case, which could plausibly make it easier for LLVM to optimise/unroll/etc the loop.

I was actually going to propose just using ptr as a index here – dereferencing a ZST is a no-op and therefore it doesn’t really matter what the pointer is, but then refrained because it is most likely UB.

It would also be worthwhile to have a FIXME somewhere to make SizeOf const-evaluated when possible, so SimplifyCfg could easily get rid of the other loop.

arielb1 · 2017-05-21T13:42:31Z

That is, the Offset could be just a Add(x, SizeOf::<T>()).

Can you do an add with LLVM pointers? Plus, I want to emit a getelementptr inbounds.

arielb1 · 2017-05-23T18:03:16Z

this should fix the segfault

Mark-Simulacrum · 2017-05-23T20:51:50Z

src/librustc_trans/mir/block.rs


        // Create the cleanup bundle, if needed.
+        let tcx = bcx.tcx();
+        let span = terminator.source_info.span;
+        let funclet_bb = match self.cleanup_kinds[bb] {


You added a method (funclet_bb) to CleanupKind that does this, shouldn't we just call it here? Unless I'm missing some detail.

umm yeah. fixed

nagisa · 2017-05-24T22:54:01Z

LGTM. I asked on IRC:

eddyb, I believe you asked for a codegen test for the arielby’s array dropping PR. Do you still want one given that the PR now uses the pointer loop when possible?

but got no response. Feel free to r- if you feel like one is still necessary.

@bors r+

bors · 2017-05-24T22:54:02Z

📌 Commit 9bfe40e has been approved by nagisa

eddyb · 2017-05-25T09:38:05Z

I replied to both @nagisa and @arielb1 but on IRC it's easy to miss a message 😆.
There is no test necessary as my point was specifically about LLVM's optimization ability.

bors · 2017-05-25T09:48:26Z

⌛ Testing commit 9bfe40e with merge 8889b73...

nagisa

Two minor nits. Looks great otherwise. r=me.

I won’t be able to look at github for a few upcoming days and won’t be able to instruct bors either.

nagisa · 2017-05-28T01:56:53Z

src/librustc_trans/mir/mod.rs

+    block_bcxs.iter_enumerated().zip(cleanup_kinds).map(|((bb, &llbb), cleanup_kind)| {
+        match *cleanup_kind {
+            CleanupKind::Funclet if base::wants_msvc_seh(ccx.sess()) => {
+                let bcx = Builder::with_ccx(ccx);


You could create this builder outside the loop, avoiding an allocation and deallocation for each iteration, I think?

nagisa · 2017-05-28T01:59:13Z

src/librustc_trans/mir/block.rs

-                    }
-                    CleanupKind::Internal { .. } => bcx.br(lltarget),
-                    CleanupKind::NotCleanup => bug!("jump from cleanup bb to bb {:?}", bb)
+        let llblock2 = |this: &mut Self, target: mir::BasicBlock| {


This could use a better name. Something like cleanup_adjusted_llblock maybe? lltarget_inner or lltarget_common could also work.

This fixes leakage on panic with arrays & slices. I am using a C-style for-loop instead of a pointer-based loop because that would be ugly-er to implement.

Fixes rust-lang#41888.

I'm not sure how well this works, but it's worth a try.

arielb1 · 2017-05-28T07:47:41Z

@bors r=nagisa

bors · 2017-05-28T07:47:42Z

📌 Commit 94f65c5 has been approved by nagisa

arielb1 · 2017-05-28T07:48:27Z

@bors r=nagisa

bors · 2017-05-28T07:48:28Z

📌 Commit 137e710 has been approved by nagisa

arielb1 · 2017-05-28T09:00:30Z

@bors r=nagisa

bors · 2017-05-28T09:00:31Z

📌 Commit ee982d4 has been approved by nagisa

bors · 2017-05-28T12:01:09Z

⌛ Testing commit ee982d4 with merge 924898f...

@eddyb

Translate array drop glue using MIR I was a bit lazy here and used a usize-based index instead of a pointer iteration. Do you think this is important @eddyb? r? @eddyb

bors · 2017-05-28T14:26:48Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nagisa
Pushing 924898f to master...

rust-highfive assigned eddyb May 11, 2017

jonas-schievink reviewed May 11, 2017

View reviewed changes

alexcrichton added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 11, 2017

nagisa reviewed May 11, 2017

View reviewed changes

eddyb reviewed May 12, 2017

View reviewed changes

arielb1 mentioned this pull request May 15, 2017

program segfaults when compiled with opt-level>0 #41888

Closed

Mark-Simulacrum reviewed May 15, 2017

View reviewed changes

nagisa reviewed May 15, 2017

View reviewed changes

arielb1 force-pushed the mir-array branch 2 times, most recently from 42e03a2 to 1d9c6d2 Compare May 15, 2017 15:56

arielb1 force-pushed the mir-array branch 2 times, most recently from c8478fc to e99766b Compare May 23, 2017 17:58

Mark-Simulacrum reviewed May 23, 2017

View reviewed changes

arielb1 force-pushed the mir-array branch from 8dd224d to ef17922 Compare May 23, 2017 21:02

arielb1 mentioned this pull request May 23, 2017

Suppress trait errors that are implied by other errors #41840

Merged

arielb1 force-pushed the mir-array branch from ef17922 to 9bfe40e Compare May 24, 2017 08:37

rust-highfive assigned nagisa and unassigned eddyb May 27, 2017

nagisa approved these changes May 28, 2017

View reviewed changes

arielb1 added 10 commits May 28, 2017 10:43

translate array drop glue using MIR

9da2aac

This fixes leakage on panic with arrays & slices. I am using a C-style for-loop instead of a pointer-based loop because that would be ugly-er to implement.

refactor trans::mir::block to trans all calls through the same code

24c1a07

address review comments

c6d0b5b

move "ADT master drop flag" logic to open_drop_for_adt_contents

68b7475

Fixes rust-lang#41888.

use Eq instead of Lt in loop

3bcd6fa

add NullOp::SizeOf and BinOp::Offset

7b295ee

fix RUST_LOG ICE caused by printing a default impl's DefId

5576770

fix loops in unwind code in MSVC

6548aef

I'm not sure how well this works, but it's worth a try.

use a pointer-based array drop loop for non-zero-sized types

162bc51

increase macro recursion limit

6adfbaf

arielb1 force-pushed the mir-array branch from 556c73c to 94f65c5 Compare May 28, 2017 07:43

arielb1 force-pushed the mir-array branch from 94f65c5 to 137e710 Compare May 28, 2017 07:47

fix translation of MSVC funclets that loop to their own start

ee982d4

arielb1 force-pushed the mir-array branch from 137e710 to ee982d4 Compare May 28, 2017 09:00

bors added a commit that referenced this pull request May 28, 2017

Auto merge of #41917 - arielb1:mir-array, r=nagisa

924898f

Translate array drop glue using MIR I was a bit lazy here and used a usize-based index instead of a pointer iteration. Do you think this is important @eddyb? r? @eddyb

bors merged commit ee982d4 into rust-lang:master May 28, 2017

bors mentioned this pull request May 28, 2017

MIR EndRegion Statements (was MIR dataflow for Borrows) #39409

Merged

dwrensha mentioned this pull request May 30, 2017

update for latest nightly rustc rust-lang/miri#172

Merged

kennytm mentioned this pull request Aug 16, 2017

incr.comp.: Crate Metadata not corresponding to a DefId is untracked. #41417

Closed

Translate array drop glue using MIR #41917

Translate array drop glue using MIR #41917

Conversation

arielb1 commented May 11, 2017

eddyb commented May 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagisa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 commented May 12, 2017 • edited

Choose a reason for hiding this comment

nagisa May 15, 2017 • edited

Choose a reason for hiding this comment

nagisa commented May 15, 2017

Choose a reason for hiding this comment

nagisa commented May 15, 2017

alexcrichton commented May 18, 2017

arielb1 commented May 18, 2017

nagisa commented May 20, 2017 • edited

arielb1 commented May 21, 2017 • edited

arielb1 commented May 23, 2017

Choose a reason for hiding this comment

arielb1 May 23, 2017 • edited

Choose a reason for hiding this comment

nagisa commented May 24, 2017

bors commented May 24, 2017

eddyb commented May 25, 2017

bors commented May 25, 2017

nagisa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 commented May 28, 2017

bors commented May 28, 2017

arielb1 commented May 28, 2017

bors commented May 28, 2017

arielb1 commented May 28, 2017

bors commented May 28, 2017

bors commented May 28, 2017

bors commented May 28, 2017

arielb1 commented May 12, 2017 •

edited

nagisa May 15, 2017 •

edited

nagisa commented May 20, 2017 •

edited

arielb1 commented May 21, 2017 •

edited

arielb1 May 23, 2017 •

edited