Un-wire `Integer`
ClosedPublic

Authored by hvr on Oct 19 2014, 1:46 PM.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
hvr updated this revision to Diff 913.Oct 19 2014, 1:46 PM
hvr retitled this revision from to Un-wire `Integer`.
hvr updated this object.
hvr edited the test plan for this revision. (Show Details)
hvr added reviewers: austin, simonpj.
simonpj edited edge metadata.Oct 20 2014, 4:10 AM

I don't understand this.

  1. The diff above shows stuff deleted, but nothing significant added. That can't be all!
  1. Why does T4801 allocate twice as much? We should understand what's going on.

But it would be Very Good to get this patch done. It's a small but worthwhile cleanup.

Is there a Trac ticket? I think there should be.

Simon

hvr added a comment.Oct 20 2014, 4:23 AM
In D351#8384, @simonpj wrote:

I don't understand this.

  1. The diff above shows stuff deleted, but nothing significant added. That can't be all!

Sorry, I should have explained my intent better: this patch here is mostly to investigate the effect of unwiring Integer with the current integer-gmp to see its effect in isolation of D82

  1. Why does T4801 allocate twice as much? We should understand what's going on.

I was hoping you may know. I suspect this must have to do with the core-representation handed around inside GHC being much heavier for small literals since the CorePrep transformation loses its ability to insert a S# constructor expression. But I know too little about that part of GHC... :-/

But it would be Very Good to get this patch done. It's a small but worthwhile cleanup.

Is there a Trac ticket? I think there should be.

I'll gladly create one if we want to go forward with this (even for the old integer-gmp)

hvr added a comment.EditedOct 20 2014, 4:26 AM

Oh, and I haven't implemented the

What is odd here is that for non-small integers we are careful to look up mkInteger in the environment (precisely so that it is not wired in). Then we stash it in the CorePrepEnv, and pass it to cvtLitInteger. What I don't understand is why we don't do exactly the same thing for S#, the data constructor for small integers. (Add a new field to CorePrepEnv for the S# data constructor.)

part yet... will do so soon.

T4801 with -O should have a single integer literal. Should not cost a lot more to compile.

T4801 without -O has lots of integer literals. If you don't look up S# (and you aren't), then yes you'll get a lot more thunks. Maybe that accounts for it. So better to do that. (Although it does constrain the library to *have* such a data constructor...)

Simon

hvr updated this revision to Diff 958.Oct 22 2014, 5:30 AM
hvr edited edge metadata.

Turn S# into a "known-key"

I must have missed something though, as the stage1 compiler fails to use S# and complains that

HC [stage 1] libraries/base/dist-install/build/GHC/List.o

Top level:
  Can't find interface-file declaration for data constructor integer-gmp-0.5.1.0:GHC.Integer.Type.S#
    Probable cause: bug in .hi-boot file, or inconsistent .hi file
    Use -ddump-if-trace to get an idea of which file caused the error

Re-running with -ddump-if-trace gives

Considering whether to load ghc-prim-0.3.1.0:GHC.Types {- SYSTEM -}
Starting fork { Declaration for mapMaybe
Loading decl for Data.Maybe.mapMaybe
updating EPS_
Considering whether to load ghc-prim-0.3.1.0:GHC.Prim {- SYSTEM -}
Considering whether to load ghc-prim-0.3.1.0:GHC.Prim {- SYSTEM -}
Considering whether to load ghc-prim-0.3.1.0:GHC.Types {- SYSTEM -}
Considering whether to load ghc-prim-0.3.1.0:GHC.Types {- SYSTEM -}
} ending fork Declaration for mapMaybe
} ending fork Rule mapMaybeList
Need decl for GHC.Integer.Type.mkInteger
Considering whether to load integer-gmp-0.5.1.0:GHC.Integer.Type {- SYSTEM -}
Reading interface for integer-gmp-0.5.1.0:GHC.Integer.Type;
    reason: Need decl for GHC.Integer.Type.mkInteger
readIFace /home/hvr/Haskell/GHC/ghc/libraries/integer-gmp/dist-install/build/GHC/Integer/Type.hi
readIFace /home/hvr/Haskell/GHC/ghc/libraries/integer-gmp/dist-install/build/GHC/Integer/Type.dyn_hi
updating EPS_
Need decl for GHC.Integer.Type.S#
Considering whether to load integer-gmp-0.5.1.0:GHC.Integer.Type {- SYSTEM -}

Top level:
  Can't find interface-file declaration for data constructor integer-gmp-0.5.1.0:GHC.Integer.Type.S#
    Probable cause: bug in .hi-boot file, or inconsistent .hi file
    Use -ddump-if-trace to get an idea of which file caused the error
hvr added a comment.Oct 22 2014, 5:35 AM

I've dumped the GHC/Integer/Type.hi via ghc-stage1 --show-iface to P26

hvr planned changes to this revision.EditedOct 22 2014, 5:49 AM

After investigating a bit more, I guess I should use

tcLookupDataCon :: Name -> TcM DataCon

rather than tcLookupGlobal. I'll try that asap

PS: turns out, tcLookupDataCon is just a wrapper around tcLookupGlobal, so this doesn't help either :-(

hvr updated this revision to Diff 962.Oct 22 2014, 8:44 AM

Add integerSDataConName to basicKnownKeyNames (as noted by @simonpj)

Now the testsuite doesn't show any effect anymore.

hvr added a comment.Oct 22 2014, 8:46 AM

one thing I'm not sure about; shall we rather store a DataCon instead of an Id in CPE for the S# constructor?

hvr added a comment.EditedOct 22 2014, 9:20 AM

Fwiw, the build failed only due to the unrelated

Unexpected failures:
  perf/compiler T3064 [stat not good enough] (normal)
hvr added inline comments.Oct 23 2014, 4:53 AM
compiler/coreSyn/CorePrep.lhs
539–541

...moreover, if a Maybe was used for the CPE-field, then we wouldn't have to test cIntegerLibraryType here anymore, but simply discriminate over Nothing and Just sDataCon

1114

I'd be tempted to make this a cpe_integerSDataCon :: Maybe DataCon instead; is there a reason to prefer an Id over a more specific Maybe DataCon?
(Nothing would then be used for integer-simple which has no single small-int constructor)

simonpj added inline comments.Oct 24 2014, 2:49 AM
compiler/coreSyn/CorePrep.lhs
1114

Herbert I agree with what you propose here. Much better to centralise the "is there a small-int contructor" question into one place.

hvr updated this revision to Diff 969.Oct 24 2014, 4:43 AM

Refactored patch to use Maybe DataCon instead of Id in CPE, as well as other cleanups

hvr added a comment.Oct 24 2014, 5:33 AM

@austin, I'm really tempted to disable T3064, as this only causes unnecessary noise :-(

Unexpected failures:
  perf/compiler T3064 [stat not good enough] (normal)
hvr updated this object.Oct 24 2014, 3:40 PM
hvr updated the Trac tickets for this revision.
austin accepted this revision.Oct 27 2014, 8:13 AM
austin edited edge metadata.

LGTM, providing @simonpj gives it the clear.

This revision is now accepted and ready to land.Oct 27 2014, 8:13 AM
This revision was automatically updated to reflect the committed changes.

Fine with content. I've made some suggestions about documenting the change.

Simon

compiler/coreSyn/CorePrep.lhs
1114

Please a Note to explain what this field is doing! Data types are such a golden opportunity for explanations.

I'd also use a different data for that (Maybe DataCon), again an opportunity for comment, and it'd be very clarifying in its uses in TidyPgm.

Simon