Changing prefetch primops to have a `seq`-like interface
ClosedPublic

Authored by carter on Oct 18 2014, 3:44 PM.

Details

Summary

The current primops for prefetching do not properly work in pure code; namely, the primops are not 'hoisted' into the correct call sites based on when arguments are evaluated. Instead, they should use a seq-like interface, which will cause it to be evaluated when the needed term is.

See Trac #9353 for the full discussion.

Test Plan

updated tests for pure prefetch in T8256 to reflect the design changes in Trac #9353

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Branch
fixedup-prefetch
Lint
Lint WarningsExcuse: ignore
SeverityLocationCodeMessage
Warningcompiler/codeGen/StgCmmPrim.hs:739TXT3Line Too Long
Warningcompiler/codeGen/StgCmmPrim.hs:744TXT3Line Too Long
Warningcompiler/codeGen/StgCmmPrim.hs:749TXT3Line Too Long
Warningcompiler/codeGen/StgCmmPrim.hs:754TXT3Line Too Long
Auto-Fixtestsuite/tests/codeGen/should_run/T8256.hs:38TXT9Trailing Whitespace at EOF
Unit
No Unit Test Coverage
Build Status
Buildable 2239
Build 2249: GHC Patch Validation (amd64/Linux)
There are a very large number of changes, so older changes are hidden. Show Older Changes
  • adjusting test suite for new type signature for pure prefetchs

looks like the only failure in the build is perf/compiler T3064 [stat not good enough] (normal)
Which is not my test! :)

that said, Luite has given me some suggestions / feedback that might require/lead to changing up the design.

ezyang removed a subscriber: ezyang.Oct 27 2014, 2:04 PM
carter updated this revision to Diff 1671.Nov 22 2014, 9:13 PM
  • updated the documention about how to use pure prefetch. also fixed a type error in the in the test
  • white space wibble
  • for the example in the documentation, replacing
  • cargo cult changing the formatting to see if that makes a difference
  • revert removing the example, also try wrapping the result in (# a #)
  • adjusting test suite for new type signature for pure prefetchs
carter updated this revision to Diff 1672.Nov 22 2014, 9:16 PM

twiddle

carter updated this revision to Diff 1677.Nov 23 2014, 12:32 AM
  • updating prefetch to have a better api

Ed, Simon, feedback would be appreciated

carter updated this revision to Diff 1679.Nov 23 2014, 12:41 AM
  • remove some deadcode from the test
  • woops, mixed up arg types in primop helpers
merijn added a subscriber: merijn.Nov 23 2014, 1:06 AM
carter updated this revision to Diff 1680.Nov 23 2014, 1:07 AM
  • fixing up some validate -werror noise
ekmett edited edge metadata.Nov 23 2014, 1:17 AM

Note unsafePerformIO is going to probably eat up any of the performance win for prefetching in the first place.

agreed, should I change the recommendation to unsafeDupable or inlinePerformIO?

carter updated this revision to Diff 1681.Nov 23 2014, 1:45 AM
carter edited edge metadata.
  • clarify documentation to make it clear that inlinePerformIO is safe to use here
carter updated this revision to Diff 1682.Nov 23 2014, 1:57 AM
  • somehow how i modified the docs made the primops file parser fail. twiddling that
carter updated this revision to Diff 1683.Nov 23 2014, 2:07 AM
  • Revert "somehow how i modified the docs made the primops file parser fail. twiddling that"
  • Revert "clarify documention to make it clear that inlinePerformIO is safe to use here"
  • clarify documentation about prefetch

i'm concerned that the prefetchValue operations dont have the right strictness information on them. The value argument shouldn't be evaluated.

carter updated this revision to Diff 1688.Nov 23 2014, 11:17 AM
  • the prefetchValue operations should be lazy in their value argument
carter updated this revision to Diff 1689.Nov 23 2014, 11:19 AM
  • use switch \ _ -> with \ _arity ->
ekmett accepted this revision.Nov 23 2014, 1:38 PM
ekmett edited edge metadata.

LGTM.

I've wanted a prefetch primop i could use to prefetch a thunk for a long time.

I have some tree processing code that can just drop in the prefetch as it takes things out of the tree and drops it in a bounded processing queue.

i was doing some sanity checking locally, and its pretty clear that the test file wasn't being run by validate somehow (as in, theres no way it compiled, i'm fixing that locally and hopefully things will then be OK)

carter updated this revision to Diff 1692.Nov 23 2014, 3:14 PM
carter edited edge metadata.
  • fixup prefetchValue0# and make the tests not depend on vector

how on earth is this validating?

carter updated this revision to Diff 1697.Nov 23 2014, 6:13 PM
  • add dcore-lint and O1 flags to test case trip the problem i've hit

i seem to have discovered a problem wrt let/app invariant with the api / semantics i have for prefetch value. However, I only seem to get the problem to happen locally on my mac (though I cant even run validate locally for other reasons i can't yet track down).

I *may* have to revert the prefetch value stuff i cant track it down

carter updated this revision to Diff 1699.Nov 23 2014, 6:57 PM
  • mark prefetchValue as has_side_effects = True

ok, marking prefetchValue has has_side_effects=True saves the day

carter updated this revision to Diff 1700.Nov 24 2014, 1:05 AM
  • adding something extra to the test suite

i think this is ready to go!

carter updated this revision to Diff 1701.Nov 24 2014, 1:59 AM
  • fixing typo in test
austin updated the Trac tickets for this revision.Nov 24 2014, 10:24 AM
carter updated this revision to Diff 1726.Nov 25 2014, 1:47 PM
  • adding has_side_effects to all the prefetch ops everywhere
carter updated this revision to Diff 1759.Nov 28 2014, 4:05 PM
  • adding more commentary about the current prefetch designs and why they need to be marked as has_side_effects=True

Davean and I did some benchmarks (mentioned in the thread here )https://www.haskell.org/pipermail/ghc-devs/2014-November/007458.html

when we ran the same codes using a version of GHC that treated the primops as being "pure", there was a teeny (but measurable) change in the resulting application performance (where the resulting performance was worse by ~ 5% of total runtime I think).

probably should add some remarks in the release notes section of the manual for this

austin requested changes to this revision.Dec 5 2014, 5:05 PM
austin edited edge metadata.

LGTM, other than the comments I have now I think.

compiler/codeGen/StgCmmPrim.hs
1555–1559

Change this comment into a regularly-styled Haddock comment and perhaps make it speak more of what the function does, rather than the abstract

-- | Translate byte array prefetch operations into proper primcalls.
doPrefetchByteArrayOp :: Int
...
1561–1564

Haddock comments needed; just add a one-line blurb here.

1579–1585

Ditto, haddock comments.

1587–1594

Haddock comments.

1596

Same here.

testsuite/tests/codeGen/should_run/T8256.hs
4–9

No need for this comment, since this test always existed for the locality primops anyway, so it doesn't need a comment log of when it might be updated.

This revision now requires changes to proceed.Dec 5 2014, 5:05 PM
carter updated this revision to Diff 1875.Dec 6 2014, 1:38 AM
carter edited edge metadata.
  • updating comments and haddocks per Austin's suggestions
carter added a comment.Dec 6 2014, 1:38 AM

did the changes as suggested.

austin accepted this revision.Dec 15 2014, 9:38 AM
austin edited edge metadata.

Ok, LGTM.

This revision is now accepted and ready to land.Dec 15 2014, 9:38 AM
austin retitled this revision from Changing the pure prefetch primops to have seqlike behavior so that prefetch can be used effectively in pure codes! This is based upon design discused on trac ticket #9353 to Changing prefetch primops to have a `seq`-like interface.Dec 15 2014, 9:42 AM
austin updated this object.
austin edited edge metadata.
This revision was automatically updated to reflect the committed changes.