avoid redundant translation of items during monomorphization #16059

spernsteiner · 2014-07-28T22:22:20Z

Currently, each time a function is monomorphized, all items within that function are translated. This is unnecessary work because the inner items already get translated when the function declaration is visited by trans_item. This patch adds a flag to the FunctionContext to prevent translation of items during monomorphization.

brson · 2014-07-28T23:09:32Z

Neat find! Are you aware of any instances where this was causing problems?

spernsteiner · 2014-07-28T23:35:54Z

It doesn't cause any visible errors, but it was making things difficult for my separate compilation branch. I thought that trans_item would be called only once on each item and relied on that property in order to get symbol linkage right, then it turned out not to be true due to this bug.

It also has the strange (but not particularly harmful) side effect of duplicating the bodies of inner functions on each monomorphization. So if f<T>() contains g(), and f is monomorphized three times, then the IR for g will contain three additional copies of each basic block (all dead, because only one of the entry_block copies can be the actual entry block of the function). These extra copies get deleted by the optimizer (even on -O0), so at worst it might increase memory usage a bit.

huonw · 2014-07-29T12:15:14Z

I wonder if this is noticable on a memory graph of the compiler, because we certainly use a lot of Vecs and HashMaps there? (cc @cmr)

dotdash · 2014-07-29T13:54:22Z

Does this fix #7349?

spernsteiner · 2014-07-29T16:15:04Z

I wonder if this is noticable on a memory graph of the compiler, because we certainly use a lot of Vecs and HashMaps there?

To be clear, I don't think this has much effect on the run-time performance of the compiled code. The compiler wastes some time and memory generating duplicate copies of various things, but the copies all get thrown away at some point. (Except maybe on Windows, where we don't use -ffunction-sections/-fdata-sections for some reason.)

Does this fix #7349?

Yeah, that looks like the same issue. With this change, the test case from your last comment there generates only one copy of the function body.

nrc · 2014-07-30T06:32:50Z

src/librustc/middle/trans/base.rs

@@ -1235,7 +1235,8 @@ pub fn new_fn_ctxt<'a>(ccx: &'a CrateContext,
                       output_type: ty::t,
                       param_substs: &'a param_substs,
                       sp: Option<Span>,
-                       block_arena: &'a TypedArena<Block<'a>>)
+                       block_arena: &'a TypedArena<Block<'a>>,
+                       ignore_items: bool)


Better to use an enum here than a bool

nrc · 2014-07-30T06:37:26Z

Is it possible to add a test to make sure this doesn't regress in the future?

spernsteiner · 2014-07-30T18:15:04Z

I changed the boolean ignore_items flag to an enum. I also added a regression test for #7349, which this PR fixes.

alexcrichton · 2014-07-30T18:36:33Z

src/test/run-make/issue-7349/foo.rs

+fn main() {
+    outer::<int>();
+    outer::<uint>();
+}


This will probably need a license to get past make tidy

alexcrichton · 2014-07-30T18:37:22Z

r=me, just some small things here and there.

spernsteiner · 2014-07-30T19:49:09Z

OK, I've fixed the issues with the test.

Currently, each time a function is monomorphized, all items within that function are translated. This is unnecessary work because the inner items already get translated when the function declaration is visited by `trans_item`. This patch adds a flag to the `FunctionContext` to prevent translation of items during monomorphization.

vadimcn · 2014-07-31T09:14:36Z

@epdtry, you've mentioned "separate compilation" above. Is this about compiling each source file into a separate object file, or about something else?

spernsteiner · 2014-08-01T22:04:04Z

@vadimcn, I'm working on allowing rustc to split a crate into several compilation units, so that it can run LLVM optimization and codegen passes on several units in parallel. The goal is to speed up compilation at the cost of a small performance hit for the generated code. It's intended for development builds, where edit-compile-test speed is particularly important.

vadimcn · 2014-08-01T22:54:04Z

@epdtry, Cool! I'd love to see that happen!
I am curious - how were you going to handle monomorphized functions? Emit them as comdats?
Would you consider writing an RFC? I imagine this would be a popular feature among Rust contributors!

spernsteiner · 2014-08-01T23:53:49Z

@vadimcn,

I am curious - how were you going to handle monomorphized functions? Emit them as comdats?

I initially had it using LLVM's linkonce_odr linkage type for monomorphizations, which I believe uses comdats internally. It didn't work out very well - it hurt compile times somewhat (LLVM has to optimize the same code in several compilation units) and link times tremendously (ld doesn't seem to appreciate being given a file with 20,000 comdats).

Nowadays, the first monomorphization (for a particular function + actual type parameters) gets defined with external linkage, and subsequent monomorphizations in other compilation units simply produce external declarations (no code, just an undefined symbol). The exception is for #[inline] functions, where the subsequent monomorphizations produce a copy of the function code with available_externally linkage. This lets LLVM do inlining using the provided definition, but produces no code in the object file (only an undefined symbol, like for external declarations). (Other #[inline] items are handled similarly - they produce an external definition in the defining compilation unit, and available_externally definitions in others.)

Would you consider writing an RFC?

I don't think it would be worth it at this point, since the project is nearly done. (The code review(s) should be up some time next week.) And anyway, I'm not sure an RFC would be useful in this case, since the only user-visible change is a pair of new codegen options in rustc.

huonw · 2014-08-02T06:43:10Z

Nowadays, the first monomorphization (for a particular function + actual type parameters) gets defined with external linkage, and subsequent monomorphizations in other compilation units simply produce external declarations (no code, just an undefined symbol).

Isn't this a backwards-compatibility concern for dynamic linkage, e.g. it is not opt-in and a crate removes its use of a certain monomorphisation that downstream crates were using? (I know we don't have a good story about dynamic linkage anyway.)

Also, #14527 seems related?

…chton Extend the changes from #16059 to the new generic foreign functions introduced by #15831.

This code produces an ICE: ```rust #![crate_type = "rlib"] fn main() { if true { return } // remaining code is unreachable match () { () => { static MAGIC: uint = 0; } } } ``` ([playpen](http://is.gd/iwOISB)) The error is "encode_symbol: id not found 18", where 18 is the `NodeId` of the declaration of `MAGIC`. The problem is that `rustc` tries to emit metadata for `MAGIC`, but some of the information is missing because `MAGIC` never gets translated by `trans_item` - the entire body of the `match` gets skipped because the `match` itself is unreachable. This branch simplifies the handling of inner items by always processing them using the `trans_item` visitor, instead of sometimes using the visitor and sometimes waiting until `trans_stmt` encounters the item. This fixes the ICE by making the translation of the item no longer depend on the declaration being reachable code. This branch also reverts #16059 and #16359, since the new change to item translation fixes the same problems as those but is simpler.

nrc reviewed Jul 30, 2014
View reviewed changes

alexcrichton reviewed Jul 30, 2014
View reviewed changes

avoid redundant translation of items during monomorphization

f97f65f

bors closed this Jul 31, 2014

bors merged commit f97f65f into rust-lang:master Jul 31, 2014

spernsteiner mentioned this pull request Aug 8, 2014

don't translate items when monomorphizing foreign-ABI functions #16359

Merged

bors added a commit that referenced this pull request Aug 9, 2014

auto merge of #16359 : epdtry/rust/mono-item-dedup-foreign, r=alexcri…

351cc4f

…chton Extend the changes from #16059 to the new generic foreign functions introduced by #15831.

spernsteiner mentioned this pull request Aug 12, 2014

fix ICE due to untranslated inner items in unreachable code #16452

Merged

luqmana mentioned this pull request Aug 19, 2014

Function body is translated more than once #7349

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

avoid redundant translation of items during monomorphization #16059

avoid redundant translation of items during monomorphization #16059

spernsteiner commented Jul 28, 2014

brson commented Jul 28, 2014

spernsteiner commented Jul 28, 2014

huonw commented Jul 29, 2014

dotdash commented Jul 29, 2014

spernsteiner commented Jul 29, 2014

nrc Jul 30, 2014

nrc commented Jul 30, 2014

spernsteiner commented Jul 30, 2014

alexcrichton Jul 30, 2014

alexcrichton commented Jul 30, 2014

spernsteiner commented Jul 30, 2014

vadimcn commented Jul 31, 2014

spernsteiner commented Aug 1, 2014

vadimcn commented Aug 1, 2014

spernsteiner commented Aug 1, 2014

huonw commented Aug 2, 2014

avoid redundant translation of items during monomorphization #16059

avoid redundant translation of items during monomorphization #16059

Conversation

spernsteiner commented Jul 28, 2014

brson commented Jul 28, 2014

spernsteiner commented Jul 28, 2014

huonw commented Jul 29, 2014

dotdash commented Jul 29, 2014

spernsteiner commented Jul 29, 2014

nrc Jul 30, 2014

Choose a reason for hiding this comment

nrc commented Jul 30, 2014

spernsteiner commented Jul 30, 2014

alexcrichton Jul 30, 2014

Choose a reason for hiding this comment

alexcrichton commented Jul 30, 2014

spernsteiner commented Jul 30, 2014

vadimcn commented Jul 31, 2014

spernsteiner commented Aug 1, 2014

vadimcn commented Aug 1, 2014

spernsteiner commented Aug 1, 2014

huonw commented Aug 2, 2014