Skip to content

Don't use allocas for immutable immediate POD values. #12424

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

Closed
wants to merge 1 commit into from

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Feb 20, 2014

Don't use allocas for immutable immediate POD values.

@nikomatsakis
Copy link
Contributor

Quick comments:

  1. Is there evidence that this enables any sort of optimization for LLVM? Compile-time improvements? I am loathe to complicate trans in this way without good cause.
  2. I don't think that Datum<Expr> is a suitable type for the locals map. If we do want to do an optimization like this, I'd prefer to add a new sort of enum other than Expr that captures precisely what modes this stuff may be in. Keeping the story about cleanup straight is rather complex and the main goal of introducing a parameterized datum type was so that the type of things like the locals array told you everything you need to know.

@nikomatsakis
Copy link
Contributor

One other comment: at least when emitting debuginfo, it is recommended to have an alloca per local variable.

@pcwalton
Copy link
Contributor

FWIW Chris Lattner has told us that we should generally err in favor of generating allocas where possible, at least for lvalues with names.

@eddyb
Copy link
Member Author

eddyb commented Feb 24, 2014

When I originally tested this, there was an reduction in both time (10s - about 2% here) and memory (30MB - it actually brought the LLVM flat line to under 1GB) used to compile stage2 librustc.

However, the generated IR is far from being nicer in all cases, because of DPS (SaveIn/Ignore), which means we output an alloca, GEPis and stores for something like an immediate (0u8, 0u8) or Some(0u8).
I might have to add support to trans::adt for generating SSA values directly.

@thestinger
Copy link
Contributor

@pcwalton: How long ago was it? There's llvm.dbg.value now for adding debug metadata for registers. It is true that clang still makes an alloca even for a const though.

@alexcrichton
Copy link
Member

Closing due to a lack of activity. It would be nice to know, though, if debug metadata does indeed work when attached to immediates instead of just alloca slots.

@alexcrichton
Copy link
Member

(belated closing)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants