Additional cleanup to rustc_trans #38756

Mark-Simulacrum · 2017-01-01T15:14:45Z

Removes BlockAndBuilder, FunctionContext, and MaybeSizedValue.

LvalueRef is used instead of MaybeSizedValue, which has the added benefit of making functions operating on Lvalues be able to take just that (since it encodes the type with an LvalueTy, which can carry discriminant information) instead of a MaybeSizedValue and a discriminant.

r? @eddyb

eddyb · 2017-01-01T15:19:29Z

src/librustc_trans/builder.rs

@@ -57,6 +79,32 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
        }
    }

+    pub fn build_new_block<'b>(&self, name: &'b str) -> Builder<'a, 'tcx> {


I'd call this build_sibling_block.

eddyb · 2017-01-01T15:20:41Z

src/librustc_trans/builder.rs

@@ -46,6 +50,24 @@ fn noname() -> *const c_char {
 }

 impl<'a, 'tcx> Builder<'a, 'tcx> {
+    pub fn entry_block(ccx: &'a CrateContext<'a, 'tcx>, llfn: ValueRef) -> Self {
+        Builder::new_block(ccx, llfn, "entry-block")
+    }


Hmm, I think it would be nice if this was actually encapsulated in define_fn but I'm not sure that actually works out well.

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

src/librustc_trans/abi.rs

@@ -295,14 +296,16 @@ impl ArgType {
        }
    }

-    pub fn store_fn_arg(&self, bcx: &BlockAndBuilder, idx: &mut usize, dst: ValueRef) {
+    pub fn store_fn_arg(
+        &self, bcx: &Builder, idx: &mut usize, dst: ValueRef


eddyb · 2017-01-02T20:32:28Z

src/librustc_trans/mir/block.rs

                } else {
-                    MaybeSizedValue::unsized_(lvalue.llval, lvalue.llextra)
+                    LvalueRef::new_unsized_ty(lvalue.llval, lvalue.llextra, ty)


These two branches could reuse lvalue and only mutate the llval field if it needs a cast.

eddyb · 2017-01-02T20:39:45Z

src/librustc_trans/mir/lvalue.rs

+        !self.llextra.is_null()
+    }
+
+    pub fn struct_field_ptr(


Is this just a helper or does this need to be public?

eddyb · 2017-01-02T20:40:15Z

src/librustc_trans/mir/lvalue.rs

                        } else {
-                            tr_base.llextra
+                            LvalueRef::new_unsized(tr_base.llval, tr_base.llextra, tr_base.ty)


Again, this is just recreating tr_base.

eddyb · 2017-01-02T21:59:37Z

src/librustc_trans/mir/block.rs

-                let args = &[ptr.llval, ptr.llextra][..1 + ptr.has_extra() as usize];
+                if bcx.ccx.shared().type_is_sized(ty) {
+                    assert!(lvalue.llextra == ptr::null_mut());
+                    if drop_ty != ty {


The two conditions can be collapsed, I believe.

eddyb · 2017-01-02T22:00:38Z

src/librustc_trans/mir/lvalue.rs

-                        let llprojected = base.trans_field_ptr(bcx, field.index());
-                        (llprojected, base.llextra)
+                        if self.ccx.shared().type_is_sized(projected_ty.to_ty(tcx)) {
+                            tr_base.llextra = ptr::null_mut();


I wouldn't mutate tr_base here but rather have a different variable (since it ends up in llextra anyway).

eddyb · 2017-01-04T01:05:02Z

src/librustc_trans/glue.rs

+                adt_def: adt_def,
+                substs: substs,
+                variant_index: variant_index,
+            };


Why not do this in the caller? Or at least at the start of the function, only once.

eddyb · 2017-01-04T01:06:34Z

src/librustc_trans/glue.rs

+                    ptr.ty = LvalueTy::Downcast {
+                        adt_def: adt,
+                        substs: substs,
+                        variant_index: Disr::from(discr).0 as usize,


How is this even a thing for AdtKind::Struct? This should never need downcasts. What is even VariantInfo?!

eddyb · 2017-01-04T01:08:21Z

src/librustc_trans/mir/block.rs

+                            ty: sig.output(),
+                        };
+                        self.store_return(&ret_bcx, ret_dest, fn_ty.ret, op);
+                        ret_bcx.position_at_end(ret_bcx.llbb());


The builder is dead after this point so it shouldn't be needed.

The builder is dead after this point so it shouldn't be needed.

I’m confused why all the repositioning is necessary at all anymore (somebody forgot to remove the hack?) if the blocks are translated in reverse-postorder.

Can we see if the stuff sstill works if both repositions are removed?

eddyb · 2017-01-04T04:26:48Z

@bors r+

bors · 2017-01-04T04:26:49Z

📌 Commit 0846c0e has been approved by eddyb

bors · 2017-01-04T08:27:19Z

⌛ Testing commit 0846c0e with merge c5c2083...

bors · 2017-01-04T08:31:14Z

💔 Test failed - status-travis

Mark-Simulacrum · 2017-01-04T14:11:59Z

Looks like a network error? LLVM's submodule couldn't be cloned.

eddyb · 2017-01-04T14:27:19Z

@bors retry

bors · 2017-01-04T16:34:33Z

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

This makes a slow transition to block construction happening only from MirContext easier.

Renames iter_variant to iter_variant_fields to more clearly communicate the purpose of the function.

Mark-Simulacrum · 2017-01-04T18:54:57Z

Rebased.

eddyb · 2017-01-04T18:57:22Z

@bors r+

bors · 2017-01-04T18:57:23Z

📌 Commit b01b6e1 has been approved by eddyb

bors · 2017-01-05T06:01:06Z

⌛ Testing commit b01b6e1 with merge 5dd07b6...

@eddyb

Additional cleanup to rustc_trans Removes `BlockAndBuilder`, `FunctionContext`, and `MaybeSizedValue`. `LvalueRef` is used instead of `MaybeSizedValue`, which has the added benefit of making functions operating on `Lvalue`s be able to take just that (since it encodes the type with an `LvalueTy`, which can carry discriminant information) instead of a `MaybeSizedValue` and a discriminant. r? @eddyb

bors · 2017-01-05T08:01:48Z

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 5dd07b6 to master...

rust-highfive assigned eddyb Jan 1, 2017

Mark-Simulacrum changed the title ~~Additional cleanup to Trans~~ Additional cleanup to rustc_trans Jan 1, 2017

eddyb reviewed Jan 1, 2017

View reviewed changes

nagisa reviewed Jan 2, 2017

View reviewed changes

eddyb reviewed Jan 2, 2017

View reviewed changes

Mark-Simulacrum force-pushed the 2nd-trans-cleanup branch from 2c76dd3 to 63790cc Compare January 2, 2017 22:11

pnkfelix added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 3, 2017

eddyb reviewed Jan 4, 2017

View reviewed changes

Mark-Simulacrum added 11 commits January 4, 2017 11:33

Replace BlockAndBuilder with Builder.

1be170b

Purge FunctionContext

937e8da

Add method, new_block, to MirContext for block construction.

f67e7d6

This makes a slow transition to block construction happening only from MirContext easier.

Add Builder::sess and Builder::tcx methods

37dd9f6

Simpliy block creation in MirContext

4c9995a

Use LvalueRef instead of MaybeSizedValue

8038489

Change trans_field_ptr to utilize LvalueTy to determine discriminant.

ea0ebe4

Move trans_const to mir::constant

982b8f4

Move trans_field_ptr and struct_field_ptr to mir/lvalue

426c558

Inline trans_switch to simplify code

81e8137

Builder.build_new_block -> Builder.build_sibling_block

901984e

Mark-Simulacrum added 7 commits January 4, 2017 11:38

Fix style nit

ba37c91

Inline and remove Builder::entry_block

c3fe259

Simplify code further

ca328e1

Remove extraneous setting of builder positions.

d25fc9e

Pull out downcasting into caller of iter_variant

7dadd14

Renames iter_variant to iter_variant_fields to more clearly communicate the purpose of the function.

Simplify handling of dropping structs.

21f86ba

Fix errors introduced during rebase

b01b6e1

Mark-Simulacrum force-pushed the 2nd-trans-cleanup branch from 0846c0e to b01b6e1 Compare January 4, 2017 18:54

bors merged commit b01b6e1 into rust-lang:master Jan 5, 2017

Mark-Simulacrum deleted the 2nd-trans-cleanup branch January 5, 2017 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Additional cleanup to rustc_trans #38756

Additional cleanup to rustc_trans #38756

Mark-Simulacrum commented Jan 1, 2017 •

edited

Loading

eddyb Jan 1, 2017

eddyb Jan 1, 2017

nagisa Jan 1, 2017

eddyb Jan 2, 2017

eddyb Jan 2, 2017

eddyb Jan 2, 2017

eddyb Jan 2, 2017

eddyb Jan 2, 2017

eddyb Jan 4, 2017

eddyb Jan 4, 2017

eddyb Jan 4, 2017

nagisa Jan 4, 2017 •

edited

Loading

eddyb commented Jan 4, 2017

bors commented Jan 4, 2017

bors commented Jan 4, 2017

bors commented Jan 4, 2017

Mark-Simulacrum commented Jan 4, 2017

eddyb commented Jan 4, 2017

bors commented Jan 4, 2017

Mark-Simulacrum commented Jan 4, 2017

eddyb commented Jan 4, 2017

bors commented Jan 4, 2017

bors commented Jan 5, 2017

bors commented Jan 5, 2017

Additional cleanup to rustc_trans #38756

Additional cleanup to rustc_trans #38756

Conversation

Mark-Simulacrum commented Jan 1, 2017 • edited Loading

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

Choose a reason for hiding this comment

nagisa Jan 4, 2017 • edited Loading

Choose a reason for hiding this comment

eddyb commented Jan 4, 2017

bors commented Jan 4, 2017

bors commented Jan 4, 2017

bors commented Jan 4, 2017

Mark-Simulacrum commented Jan 4, 2017

eddyb commented Jan 4, 2017

bors commented Jan 4, 2017

Mark-Simulacrum commented Jan 4, 2017

eddyb commented Jan 4, 2017

bors commented Jan 4, 2017

bors commented Jan 5, 2017

bors commented Jan 5, 2017

Mark-Simulacrum commented Jan 1, 2017 •

edited

Loading

nagisa Jan 4, 2017 •

edited

Loading