A interesting wrinkle! I agree with matt that having class methods show up in the profile can be very helpful at times. Constructors and their wrappers on the other hand should be of no concern. We explicitly try to avoid wrapping them in cost centres as they generally add no useful information to profiles. I've briefly looked over the code and adding class methods seems to be the *first* thing tidy does, not at it's end. (in tidyProgram) That would suggest to me that there is a need to tidy these implicit bindings so moving them might have further unintended consequences. But then Note [Injecting implicit bindings] claims we do it at the end of tidy, but seems outdated.This seems like a proper mess. If you are sure there is no need to tidy the class method selectors then it would be clearest to have a explicit "materializeImplicitBinds" step in the pipeline after tidy and before lateCC runs. That seems like it would be clearer and more coherent overall. And it would sidestep this issue completely. Cheers Andreas Am 07/04/2025 um 11:08 schrieb Matthew Pickering:

Hi Simon,

It does seem strange (at first glance) that these implicit things happen in two different places.

However, I think you should do this refactoring in a separate MR to !10479, it seems unrelated to the purpose of that patch and affects other things like you have mentioned.

Why do you think the change in callstack is fine? It seems the difference is `callstack001` is whether there an entry for `>>=` or not, which seems like quite an important thing to place a cost centre on? Andreas has a better intuition for this than me. I would quite like to know if there is an unoptimised call to >>= in my program. You have probably thought about this a little, so interested to hear your opinion.

Cheers,

Matt

On Fri, Apr 4, 2025 at 10:44 PM Simon Peyton Jones wrote:

Hi Matthew, Andreas, Ben

In GHC today there is a strange inconsistency:

* Bindings for data constructor wrappers and class method selectors are added at the end of CoreTidy. * Bindings for data constructor workers are added at the start of CorePrep

This is deeply strange.  In !10479 (unary class patch) https://gitlab.haskell.org/ghc/ghc/-/merge_requests/10479 I had to make some changes in this area and after being baffled I moved all these implicit bindings to one place at the start of CorePrep.  That is simpler and neater.  There is no reason for them to be separate.

BUT I find that one thing happens between Tidy and Prep, namely late-cost-centre injection.  That means we will no longer get late-cost-centres for data constructor wrappers and class method selectors.  I think that this is what is causing diffs in the output for callstack001 and friends in !10479.

I think that this is perfectly fine, and I propose to accept the changes.

Another alternative would be to inject all of these bindings at the end of Tidy (instead of the start of Prep),  But it's much nicer at the start of Prep -- we don't want any of these bindings to appear  in interface files.

Can you confirm that the change is ok?  Thanks

Simon

_______________________________________________ ghc-devs mailing list ghc-devs@haskell.org http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

But then we'll add cost centres to data constructor workers and wrappers. ... but have the "add cost centre" pass skip over data constructor workers and wrappers.

I'm a bit inclined to combine the late-cc/materialise/prep combo into

We already do this. See compiler/GHC/Core/LateCC/TopLevelBinds.hs:112 where we already check for constructor bindings. I've added this to avoid adding ccs to workers but it should work for wrappers just the same. That means I think your plan A should work fine, assuming there is no need to tidy those binds. the main function of GHC.CoreToStg.Prep. In general I always found it easier to understand passes when they are narrow in scope. Sometimes combining them is worth it for performance reasons but I suspect this is not so here. And their concerns are clearly separate so combining them seems odd to me from code structure point of view as well. For example LateCC must return a list of inserted cost centres, while CorePrep doesn't need to care about such a thing at all currently! So I would rather keep those clearly separated.

I was thinking that the class-op selector is a small, cheap function, so I don't mind not profiling it. But perhaps your point is that calling it symptomatic of running overloaded code, and we might want to know that.

It has definitely been a helpful indicator that optimizations went wrong for me in the past, as one wouldn't expect (>>=) or similar to be called 5 million times in a optimized program. Beyond that one of the ideas behind the current late cost centre profiling is that it can tell you with fairly high precision which (top level) functions are actually executed at runtime. Removing method selectors muddies the water there a bit. Am 07/04/2025 um 13:37 schrieb Simon Peyton Jones:

If you are sure there is no need to tidy the class method selectors then it would be clearest to have a explicit "materializeImplicitBinds" step in the pipeline after tidy and before lateCC runs.

Yes, I want to put all the materialisation in one place.  I think you are suggesting Plan A

* Tidy * Materialise implicit bindings (class op selectors, data constructor workers, data constructor wrappers) * Add late cost centres * Run late plugins * Prep * CoreToStg

But then we'll add cost centres to data constructor workers and wrappers.

Or Plan B

* Tidy * Add late cost centres * Materialise implicit bindings (class op selectors, data constructor workers, data constructor wrappers * Run late plugins * Prep * CoreToStg

Now we won't get cost centres on class-op selectors, which Matthew says he wants.

I suppose we could have do Plan A, but have the "add cost centre" pass skip over data constructor workers and wrappers.

Do you have any views?

I'm a bit inclined to combine the late-cc/materialise/prep combo into the main function of GHC.CoreToStg.Prep.   But I wonder if the "late plugins" need to see (a) the materialised implicit bindings and (b) late cost centres.    I have no idea how late plugins are used.

Matthew asks

I would quite like to know if there is an unoptimised call to >>= in my program. You have probably thought about this a little, so interested to hear your opinion.

I was thinking that the class-op selector is a small, cheap function, so I don't mind not profiling it.  But perhaps your point is that calling it symptomatic of running overloaded code, and we might want to know that.

Simon

On Mon, 7 Apr 2025 at 10:37, Andreas Klebinger wrote:

A interesting wrinkle!

I agree with matt that having class methods show up in the profile can be very helpful at times.

Constructors and their wrappers on the other hand should be of no concern. We explicitly try to avoid wrapping them in cost centres as they generally add no useful information to profiles.

I've briefly looked over the code and adding class methods seems to be the *first* thing tidy does, not at it's end. (in tidyProgram) That would suggest to me that there is a need to tidy these implicit bindings so moving them might have further unintended consequences.

But then Note [Injecting implicit bindings] claims we do it at the end of tidy, but seems outdated.This seems like a proper mess.

If you are sure there is no need to tidy the class method selectors then it would be clearest to have a explicit "materializeImplicitBinds" step in the pipeline after tidy and before lateCC runs. That seems like it would be clearer and more coherent overall. And it would sidestep this issue completely.

Cheers Andreas

Am 07/04/2025 um 11:08 schrieb Matthew Pickering:

...

Hi Simon,

It does seem strange (at first glance) that these implicit things happen in two different places.

However, I think you should do this refactoring in a separate MR to !10479, it seems unrelated to the purpose of that patch and affects other things like you have mentioned.

Why do you think the change in callstack is fine? It seems the difference is `callstack001` is whether there an entry for `>>=` or not, which seems like quite an important thing to place a cost centre on? Andreas has a better intuition for this than me. I would quite like to know if there is an unoptimised call to >>= in my program. You have probably thought about this a little, so interested to hear your opinion.

Cheers,

Matt

On Fri, Apr 4, 2025 at 10:44 PM Simon Peyton Jones wrote:

Hi Matthew, Andreas, Ben

In GHC today there is a strange inconsistency:

* Bindings for data constructor wrappers and class method selectors are added at the end of CoreTidy. * Bindings for data constructor workers are added at the start of CorePrep

This is deeply strange.  In !10479 (unary class patch) https://gitlab.haskell.org/ghc/ghc/-/merge_requests/10479 I had to make some changes in this area and after being baffled I moved all these implicit bindings to one place at the start of CorePrep.  That is simpler and neater.  There is no reason for them to be separate.

BUT I find that one thing happens between Tidy and Prep, namely late-cost-centre injection.  That means we will no longer get late-cost-centres for data constructor wrappers and class method selectors.  I think that this is what is causing diffs in the output for callstack001 and friends in !10479.

I think that this is perfectly fine, and I propose to accept the changes.

Another alternative would be to inject all of these bindings at the end of Tidy (instead of the start of Prep),  But it's much nicer at the start of Prep -- we don't want any of these bindings to appear  in interface files.

Can you confirm that the change is ok? Thanks

Simon

_______________________________________________ ghc-devs mailing list ghc-devs@haskell.org http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs