- User Since
- Jun 8 2014, 1:59 AM (163 w, 3 d)
Mon, Jul 24
We should probably revisit that...
Fri, Jul 21
Thu, Jul 20
Tue, Jul 18
Didn't read it all in detail, but in principle this looks fine. Please fix the long lines before committing.
Mon, Jul 17
I think this is OK, none and ignore are different use cases, although perhaps ignore is generally more useful.
Wed, Jul 12
Yep, that oughta do it.
Mon, Jul 10
Ok, this looks better without mgHead and mgReverse.
Wed, Jul 5
Agree, let's not do this.
Tue, Jul 4
Mon, Jul 3
Fri, Jun 30
Ah, I was already working on changes to the user manual that we could just link to from the release notes. Better to have this in one place I think, and we need it in the manual proper.
Thu, Jun 29
Wed, Jun 28
I don't think it matters, the fix for Trac #13242 was the right thing anyway, but now we don't need to call out existentials in the docs because the problem doesn't arise. I'm just revising the docs so I'll do this.
add xref to wiki
FYI, the invariants are actually pretty well documented here: https://ghc.haskell.org/trac/ghc/wiki/Commentary/Rts/HaskellExecution/PointerTagging (I wish I'd checked that earlier)
I'd forgotten why we might have a WHITEHOLE here, and I think the only reason is that lockCAF uses it temporarily while entering a CAF. Normal non-CAF BLACKHOLEs can never be WHITEHOLE, I believe.
Jun 26 2017
Thanks @bgamari, I hadn't noticed that we enabled this by default.
The suggestion was probably misguided, just ignore it.
Jun 22 2017
Jun 21 2017
Ah, I think this will only work with the non-threaded RTS, and we can't easily test that because we'd have to relink GHCi. Sorry about that! Just remove the test and we're good to go.
Jun 19 2017
@AaronFriel the issue is that type error messages will display the transformed code, and not the original code that the user wrote. You'll see this if you construct an example that (a) is subject to the new transformation introduced by your patch, and (b) contains a type error that includes the transformed code. ApplicativeDo as currently implemented avoids this problem because when pretty-printed, ApplicativeStmt looks exactly like the original source code. If we turn a BodyStmt into a BindStmt, then we lose that property. Does that help?
So I agree there's a bug here, but I'm afraid I don't understand the fix!
Jun 16 2017
We're good to go here. I talked to @simonpj and we agreed that this direction is fine for now, in the future we can think about redesigning hoopl and once again extracting it from GHC.
Jun 15 2017
The title says WIP, but I'm not sure what's still IP here. Seems reasonable so I'll accept anyway.
Hmm. I'm not really keen on the design here, particularly the way that ModuleGraph contains both a [ModSummary] and also the ModuleEnvs. Can we make ModuleGraph not keep the [ModSummary]? Perhaps topSortModuleGraph could return [ModSummary], not ModuleGraph. Or perhaps it should really contain a graph, so that you can get the top-sorted order any time you like.
- Do we want to do this? I think we should, being careful to provide a smooth migration path for existing users. Let's discuss how exactly to do this before any changes take place.
Jun 13 2017
I think you need to adjust the bounds on the perf tests again. Did T13379 regress?
@bgamari please merge this to the 8.2 branch if you'd be so kind!
Jun 12 2017
Jun 11 2017
Jun 8 2017
The naming looks much better.
Jun 7 2017
indentation & lint
Jun 1 2017
Are the test failures relevant?
This looks OK in general, but somehow I feel like there's a more general solution trying to get out.
Sure, looks good to me. Does it make any difference to the perf tests?
May 30 2017
I won't object, other than to note that this is typically the opposite direction that we normally want to go in, that is to use third-party libraries via their normal APIs rather than to fork them. We're trying to go in the other direction with pretty, for example. Having said that, I know it's difficult to reconcile some of the divergence without losing performance, particularly the manual specialisation. But some of the other impedence mismatch could be worked around, by either unifying APIs or centralising a renaming layer.
Looks right to me, just need to fix the copy/pasto pointed out by @alexbiehl
May 24 2017
Nice, I'll leave it for @simonpj to accept, just nits and style suggestions from me.
May 23 2017
-pie seems reasonable to me. But couldn't -shared be used for this? There's an arguable consistency to it: -shared makes a dynamically-loadable library, it would be nice if it did the same thing for an executable.
May 22 2017
May 18 2017
Ok, passing back to you for refactoring. I'll take another look when you update the diff.
May 17 2017
Ok, you're using a different test case now. It looks fine, but you didn't show the output of a type error message, which I think will demonstrate the problem.
May 16 2017
May 15 2017
We don't have a good way to virtualize the file system (or IO in general). My concern is that whatever mechanisms we put in place here won't be enough.
@simonmar, what do you think about this?
Nice observation about the unnecessary reloads, and the results look good. I presume with -O the unnecessary reloads will be eliminated by CmmSink, but eliminating them earlier saves compile time and helps with -O0.
If it works on Windows, go for it.
May 11 2017
I think in your test case ApplicativeDo is not transforming the statement. You need to test it with an example that gets transformed after your diff.
I really don't like this, let's try to find alternative solutions.
May 10 2017
To be clear I don't object to a stop-gap approach that allows BodyStmt to work with ApplicativeDo without going all the way to Trac #10892, but the first point in my previous comment still applies: I think this will break error messages. I'm not sure what the best way to fix this is so I'll let you experiment, but we have to retain the information somehow that the original statement was a BodyStmt so that it can be printed as such in an error message.
May 9 2017
Ok, so there are a couple of problems with this approach
May 8 2017
@kavon should have a look at this too
May 5 2017
Well ok, if you can demonstrate a perf improvement then I won't object. GHC does use unsafeInterleaveIO, but as far as I know we've never identified it as a significant factor for performance (that could be because our profiling tools aren't good enough, of course). So my concerns are really around whether this is the most productive area to focus effort, rather than whether it's a good idea in isolation.
May 4 2017
I think I benchmarked this when I was working on the backend and found that it was worse. What effect did it have on the perf tests?
May 3 2017
@dfeuer what's this aiming at? Is there a problem with the old implementation, or are you trying to optimise it? If so, what's the benchmark?
Looks like it should be ok, although this is an unforced change so I feel slightly uneasy about it. Do we have good enough tests to be confident in this do you think?