Don't inline top-level primitive string literals
AbandonedPublic

Authored by niteria on Feb 19 2018, 11:15 AM.

Details

Summary

I've explained the rationale in the comment, please correct me if
I'm wrong.

One thing that I'm not sure about is if primitive string literals
can be transformed in meaningful ways by RULES and if leaving them in context
would help there. I can imagine someone who wants to convert primitive
strings between UTF8 and UTF-16 at compile time.

Test Plan

CI

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Branch
ghc-HEAD-dwarf-lint-fixes
Lint
Lint WarningsExcuse: adhere to file style
SeverityLocationCodeMessage
Warningcompiler/simplCore/SimplUtils.hs:1123TXT3Line Too Long
Unit
No Unit Test Coverage
Build Status
Buildable 19593
Build 41530: [GHC] Linux/amd64: Continuous Integration
Build 41529: [GHC] OSX/amd64: Continuous Integration
Build 41528: [GHC] Windows/amd64: Continuous Integration
Build 41527: arc lint + arc unit
niteria created this revision.Feb 19 2018, 11:15 AM
niteria edited the summary of this revision. (Show Details)Feb 19 2018, 11:17 AM
niteria updated this revision to Diff 15506.Feb 19 2018, 12:14 PM

update test output

I run nofib a couple of time and it's hard to see anything among the noise P175. I don't *think* I have cpu scaling on, is usually less noisy for other people?

I run nofib a couple of time and it's hard to see anything among the noise P175. I don't *think* I have cpu scaling on, is usually less noisy for other people?

@nomeata uses Cachegrind to run nofib on Gipeda to reduce the noise (at the expense of measuring a proxy for time rather than time itself). Maybe he can offer some suggestions.

dfeuer added inline comments.Feb 19 2018, 5:48 PM
compiler/simplCore/SimplUtils.hs
1101

Grammar and punctuation suggestions:

We don't inline top-level primitive strings; instead, we purposefully float primitive
string literals to the top. Inlining them wouldn't help with any optimizations;
it can only prevent other expressions from getting inlined by making
them look big. See also Note [CoreSyn top-level string literals].

Question: is there a difference here between "primitive strings" and "primitive string literals"? Why do we inline literal strings that are not top-level? The same question actually applies to bottoming Ids; perhaps one Note could explain that aspect of both.

bgamari added inline comments.Feb 19 2018, 7:49 PM
compiler/simplCore/SimplUtils.hs
1101
Is there a difference here between "primitive strings" and "primitive string literals"?

I believe both just refer to literal expressions of type Addr# e.g.

x :: Addr#
x = "hello world!"#

Why do we inline literal strings that are not top-level?

All primitive stringsliterals are floated to the top-level, so there shouldn't be any non-top-level primitive string literals.

dfeuer added inline comments.Feb 19 2018, 8:24 PM
compiler/simplCore/SimplUtils.hs
1101

So why the isTopLevel test?

simonpj added a comment.EditedFeb 20 2018, 5:10 AM

What is the motivation for this change? Is there a Trac ticket for it?

The change only affects bindings like

str = "foo"#

x :: Int
x = lengthStr str    -- Assuming lengthStr :: Addr# -> Int

Currently we'll do preInlineUnconditionally on str to get

x = lengthStr "foo"#

Question: what is wrong with doing that? It's also OK *not* to do it, but it's extra code and I do not yet see a motivation.

Your comment says "inlining the literal may make something look big" but here x is a redex, so it won't be inlined anyway. Unless it occurs exactly once, in which case it'll be inlined regardless of size.

We need a good reason to add extra code!

niteria added a comment.EditedFeb 20 2018, 5:34 AM

What is the motivation for this change? Is there a Trac ticket for it?

I've noticed it when looking at Trac #14779. I don't claim it fixes any existing problem.
I just think it makes sense not to undo floating that we purposefully do.
I assumed this case is analogous to bottoming Ids.

I think I assume a couple of things, some of which may be incorrect:

  1. the simplifier floats every primitive string to the top
  2. inlining it back where it was floated from won't let us optimize the program better
  3. we have primitive strings flip-flopping between being inlined and floated, which seems like a waste of effort

I'm happy to abandon this change, it doesn't help me in any significant way. I hoped this would be an obvious improvement, but it looks like things are more nuanced.

AndreasK added a subscriber: AndreasK.EditedFeb 20 2018, 6:34 AM

I run nofib a couple of time and it's hard to see anything among the noise P175. I don't *think* I have cpu scaling on, is usually less noisy for other people?

Some noise is to be expected. Doing more runs can help reduce it but changes <1% hard pretty hard to measure nevertheless. If you use NoFibRuns=30 you should be able to get the noise to less than 1%.

What is the motivation for this change? Is there a Trac ticket for it?

I've noticed it when looking at Trac #14779. I don't claim it fixes any existing problem.
I just think it makes sense not to undo floating that we purposefully do.
I assumed this case is analogous to bottoming Ids.

I think I assume a couple of things, some of which may be incorrect:

  1. the simplifier floats every primitive string to the top
  2. inlining it back where it was floated from won't let us optimize the program better
  3. we have primitive strings flip-flopping between being inlined and floated, which seems like a waste of effort

    I'm happy to abandon this change, it doesn't help me in any significant way. I hoped this would be an obvious improvement, but it looks like things are more nuanced.

I haven't looked into it in detail but could that also help with Trac #14687 if expanded to Ints?

Currently the inlining causes the Int to be recreated on the HEAP. (The example on there might only appear on Windows as Windows lacks the small int optimization).
This means a heap check and more GC pressure which we could then avoid.

I haven't looked into it in detail but could that also help with Trac Trac #14687 if expanded to Ints?

I don't think this has to do with Trac Trac #14687. The issue in this Phab is solely special-caseing literal strings

niteria abandoned this revision.Feb 22 2018, 7:05 AM