Allow packing constructor fields
ClosedPublic

Authored by michalt on Jul 31 2017, 1:20 PM.

Details

Summary

This is another step for fixing Trac #13825 and is based on D38 by Simon
Marlow.

The change allows storing multiple constructor fields within the same
word. This currently applies only to Floats, e.g.,

data Foo = Foo {-# UNPACK #-} !Float {-# UNPACK #-} !Float

on 64-bit arch, will now store both fields within the same constructor
word. For WordX/IntX we'll need to introduce new primop types.

Main changes:

  • We now use sizes in bytes when we compute the offsets for constructor fields in StgCmmLayout and introduce padding if necessary (word-sized fields are still word-aligned)
  • ByteCodeGen had to be updated to correctly construct the data types. This required some new bytecode instructions to allow pushing things that are not full words onto the stack (and updating Interpreter.c). Note that we only use the packed stuff when constructing data types (i.e., for PACK), in all other cases the behavior should not change.
  • RtClosureInspect was changed to handle the new layout when extracting subterms. This seems to be used by things like :print. I've also added a test for this.
  • I deviated slightly from Simon's approach and use PrimRep instead of ArgRep for computing the size of fields. This seemed more natural and in the future we'll probably want to introduce new primitive types (e.g., Int8#) and PrimRep seems like a better place to do that (where we already have Int64Rep for example). ArgRep on the other hand seems to be more focused on calling functions.

Signed-off-by: Michal Terepeta <michal.terepeta@gmail.com>

Test Plan

./validate

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.
michalt created this revision.Jul 31 2017, 1:20 PM
maoe added a subscriber: maoe.Aug 1 2017, 4:53 PM
bgamari requested changes to this revision.Aug 18 2017, 8:17 AM

Bravo! This looks quite good. Just a few comments.

@simonmar, do you think you could have a look over as well when you get a chance?

compiler/ghci/ByteCodeInstr.hs
315

It might be a good idea to describe the "all stack items are one word" invariant described above to a Note and reference it here.

compiler/types/TyCon.hs
1355

Why do we have wORD64_SIZE and yet not fLOAT_SIZE?

This revision now requires changes to proceed.Aug 18 2017, 8:17 AM
simonmar edited edge metadata.Aug 21 2017, 3:00 AM

This looks great, thanks for doing all the extra work to tidy this up and get it in a commitable state!

I think a few more tests would be in order. We should have tests that the compiled backend uses the correct representation, and that we can successfully share packed data built by the interpreter with compiled code and vice versa.

compiler/codeGen/StgCmmLayout.hs
396–403

Seems odd for one of these to be an offset and the other to be a length. This isn't the way I did it in D38, there we had:

( Either               -- Either:
     ByteOff           --   Some padding bytes
     (NonVoid a)       --   A real field
, ByteOff)

I think the order of the field in the output must match the order of the inputs (that should probably be stated in a comment).

compiler/ghci/ByteCodeGen.hs
1568–1570

I think we should have PUSH_PAD instructions for these, so we don't have to make dummy literals.

compiler/ghci/ByteCodeInstr.hs
97–101

I think we could probably include the value directly in the instruction rather than putting it in the literal pool for these instructions. (no need to do it in this diff, could be a future improvement)

michalt updated this revision to Diff 13949.Sep 17 2017, 10:38 AM
michalt edited edge metadata.
michalt marked 3 inline comments as done.

Address comments

Thanks for comments @bgamari and @simonmar :)

This looks great, thanks for doing all the extra work to tidy this up and get it in a commitable state!

I think a few more tests would be in order. We should have tests that the compiled backend uses the correct representation, and that we can successfully share packed data built by the interpreter with compiled code and vice versa.

I've added a test that should test the interoperability between compiled and interpreted code (let me know if you see an easier/better way to do that!)

However, I'm not sure I know how exactly to test that "the compiled backend uses the correct representation". For code like that I'd usually just write some unit tests (or quickcheck properties), but that doesn't seem to be the usual way of testing GHC? For now I've added a few similar tests as for GHCI but with compile_and_run.

compiler/codeGen/StgCmmLayout.hs
396–403

You're right - this is a bit awkward. I initially had Padding ByteOff ByteOff to have *both* the offset and length (which is basically the same as the pair from D38 was doing), but the offset wasn't actually used anywhere... I'll re-add it to make this code less surprising/confusing.

Let me know if you had something else in mind!

compiler/ghci/ByteCodeInstr.hs
97–101

I'm not sure I understand your suggestion.. AFAICS we'll still want to support multiple different types of small primitives. Currently there is only Float# but in the future I'd like to also have things like Int8#, etc. Are you saying we should bitcast everything to Word* and keep that in the instruction?

315

These are actually overapproximations - the stuff that we put for PACK will take less than a word. I've added a few comments.

compiler/types/TyCon.hs
1355

I don't know :)

I've added a definition next to wORD64_SIZE.

However, I'm not sure I know how exactly to test that "the compiled backend uses the correct representation". For code like that I'd usually just write some unit tests (or quickcheck properties), but that doesn't seem to be the usual way of testing GHC? For now I've added a few similar tests as for GHCI but with compile_and_run.

Adding a testcase to the testsuite with a pile of unit tests in it sounds perfectly reasonable to me.

You might find this (https://phabricator.haskell.org/D2903#7c4e0dd1) to be a helpful example of how to unit-test GHC.

michalt updated this revision to Diff 14030.Sep 21 2017, 12:30 PM

Add some unit tests

You might find this (https://phabricator.haskell.org/D2903#7c4e0dd1) to be a helpful example of how to unit-test GHC.

Thanks, that was useful! :)

Btw. I have a question regarding wORD_SIZE - we currently use it for computing the offsets and I assumed that this is the word size of the *target* platform (i.e., what we're compiling for). But is that correct? It seems like it's hardcoded in PlatformConstants and is different than sTargetPlatform (in DynFlags)...

Sorry for the late response! I've fallen a bit behind in reviews recently.

Btw. I have a question regarding wORD_SIZE - we currently use it for computing the offsets and I assumed that this is the word size of the *target* platform (i.e., what we're compiling for). But is that correct? It seems like it's hardcoded in PlatformConstants and is different than sTargetPlatform (in DynFlags)...

Indeed wORD_SIZE is the word size of the target. Note that PlatformConstants is generated by deriveConstants, which indeed derives constants for the target. Specifically, the compiler reads the PlatformConstants value from the platformConstants file included in GHC's lib/ directory.

Tests look good to me.

bgamari accepted this revision.Oct 25 2017, 1:31 PM

Alright, it sounds like we are ready to merge this in that case.

This revision is now accepted and ready to land.Oct 25 2017, 1:31 PM
This revision was automatically updated to reflect the committed changes.