#8742: Reuse scavenge_small_bitmap -------------------------------------+------------------------------------ Reporter: Tarrasch | Owner: simonmar Type: task | Status: patch Priority: lowest | Milestone: 7.10.1 Component: Runtime System | Version: 7.6.3 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Unknown/Multiple Type of failure: None/Unknown | Difficulty: Unknown Test Case: | Blocked By: Blocking: | Related Tickets: -------------------------------------+------------------------------------ Changes (by simonmar): * milestone: => 7.10.1 Comment: yup. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8742#comment:2 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#8742: Reuse scavenge_small_bitmap -------------------------------------+------------------------------------ Reporter: Tarrasch | Owner: Type: task | Status: new Priority: lowest | Milestone: 7.10.1 Component: Runtime System | Version: 7.6.3 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Unknown/Multiple Type of failure: None/Unknown | Difficulty: Unknown Test Case: | Blocked By: Blocking: | Related Tickets: -------------------------------------+------------------------------------ Changes (by simonmar): * owner: simonmar => * status: patch => new Comment: Sure. Just update the patch here to include those too. If you could check that the generated code is exactly the same that would be good. I suspect I inlined these things by hand out of paranoia. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8742#comment:4 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#8742: Reuse scavenge_small_bitmap -------------------------------------+------------------------------------ Reporter: Tarrasch | Owner: Type: task | Status: new Priority: lowest | Milestone: 7.10.1 Component: Runtime System | Version: 7.6.3 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Unknown/Multiple Type of failure: None/Unknown | Difficulty: Unknown Test Case: | Blocked By: Blocking: | Related Tickets: -------------------------------------+------------------------------------ Comment (by Tarrasch): Unfortunately, the output object files did differ. I even did `objdump --source rts/dist/build/sm/Scav.o` and saw that the assembly output actually was different before and after the patch. I would guess that gcc isn't smart enough to in-line a usage like this: `my_var = inline_function(..., my_var, ...)`. It probably doesn't realize that there's no need to save `my_var` in a temporary. Should we abandon this ticket or is it worth merging anyway? I don't have any good judgement here since I'm not sure how hot this code is. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8742#comment:6 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#8742: Reuse scavenge_small_bitmap -------------------------------------+------------------------------------ Reporter: Tarrasch | Owner: Type: task | Status: new Priority: lowest | Milestone: 7.10.1 Component: Runtime System | Version: 7.6.3 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Unknown/Multiple Type of failure: None/Unknown | Difficulty: Unknown Test Case: | Blocked By: Blocking: | Related Tickets: -------------------------------------+------------------------------------ Comment (by Tarrasch): Nevermind, I'm just forgetting that I'm stupid. I spent a lot of time experimenting and trying to create a minimal test-case where gcc isn't able to inline. Tough it turned out that my premise was wrong all along, it isn't equivalent to inline the code due to the implicit type casts. It so happens that in `scavenge_arg_block` and `scavenge_stack` the `size` argument is of type `nat`. For these two cases the `STATIC_INLINE` and explicit inline methods created exactly identical object files. But for the function `scavenge_PAP_payload` the variable (actually parameter) with the same name `size` had type `StgWord`. To make this even worse confusing, the passed argument to `scavenge_PAP_payload` is of type `StgHalfWord`. Phew. My suggested resolution is to first apply a patch changing the type signature of `scavenge_PAP_payload` to take a `nat` instead of an `StgWord`. I'll attach a patch. After that the original patch should be merge-able without changing the output of the object files. :) ---- Oh god, I wasted *so much time* on this, for what it's worth, here are some gists I produced in the meantime: https://gist.github.com/Tarrasch/8965562 https://gist.github.com/Tarrasch/8966086 https://gist.github.com/Tarrasch/8971992 https://gist.github.com/Tarrasch/8972574 -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8742#comment:8 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#8742: Reuse scavenge_small_bitmap -------------------------------------+------------------------------------ Reporter: Tarrasch | Owner: Type: task | Status: patch Priority: lowest | Milestone: 7.10.1 Component: Runtime System | Version: 7.6.3 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Unknown/Multiple Type of failure: None/Unknown | Difficulty: Unknown Test Case: | Blocked By: Blocking: | Related Tickets: -------------------------------------+------------------------------------ Comment (by Tarrasch): Ok. I stumbled upon the same exact set of issues with non-matching types for `Compact.c`. It seems like at differenct places `StgWord`, `W_` and `nat` is used. And again `scavenge_PAP_payload` gets passed `StgHalfWord`s. If I were to unify these to use the same type for the variable `size`, which should it be? I would myself just go for `StgWord` since if you look at the `bitmap` variable, it's usually of type `StgWord`, and usually the equation is that `size == BITMAP_SIZE(bitmap) == ((bitmap) & BITMAP_SIZE_MASK)`. Am I over-thinking this? I just think that with these series of patches we get these two nice benefits: 1. A consistent use of the types (or is there any good reason they differ so much?) 2. We can safely apply two patches that would remove a total of 30 lines of code. Without worrying that we loose performance since the `.o` files can be checked to be exact matches. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8742#comment:10 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#8742: Reuse scavenge_small_bitmap -------------------------------------+------------------------------------ Reporter: Tarrasch | Owner: Type: task | Status: patch Priority: lowest | Milestone: 7.10.1 Component: Runtime System | Version: 7.6.3 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Unknown/Multiple Type of failure: None/Unknown | Difficulty: Unknown Test Case: | Blocked By: Blocking: | Related Tickets: -------------------------------------+------------------------------------ Comment (by Tarrasch): Hello Simon, I'll be more than happy to change the uses of `size` to use type `StgWord`, tough I start realizing that such patch can become quite big if I go outside of `Scav.c`. For example, in `Compact.c`, there is a usage of `closure_sizeW_` which have a return type of `nat`. My best judgement says that I should change it to return of type `StgWord`, but since it's such a big and used function it might affect other parts of the code. Am I in the right direction? Regardless I think that this effort to unify the types is beneficial, because at many of these in-lined call sites there are implicit type conversions which cost cycles performance. ''We'' know that the `size` will always fit in 32 bits, but gcc doesn't. So, would a patch including a change of `nat closure_sizeW_` to `StgWord closure_sizeW_` be a good idea? -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8742#comment:12 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#8742: Reuse scavenge_small_bitmap -------------------------------------+------------------------------------ Reporter: Tarrasch | Owner: Tarrasch Type: task | Status: patch Priority: lowest | Milestone: 7.10.1 Component: Runtime System | Version: 7.6.3 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Unknown/Multiple Type of failure: None/Unknown | Difficulty: Unknown Test Case: | Blocked By: Blocking: | Related Tickets: -------------------------------------+------------------------------------ Comment (by simonmar): Don't go crazy here (also, it's not on your critical path!). Yes it would be good to change uses of `nat` to `StgWord`, but there's no hurry. If you want to do that as a separate patch then go ahead. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8742#comment:14 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#8742: Reuse scavenge_small_bitmap -------------------------------------+------------------------------------ Reporter: Tarrasch | Owner: Tarrasch Type: task | Status: patch Priority: lowest | Milestone: 7.10.1 Component: Runtime System | Version: 7.6.3 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Unknown/Multiple Type of failure: None/Unknown | Difficulty: Unknown Test Case: | Blocked By: Blocking: | Related Tickets: -------------------------------------+------------------------------------ Comment (by Tarrasch): Hi Simon, Did you have time to look at the "unification" patch I sent? I did not measure its change to performance and it will naturally change the output assembly (but I do not know if for better or for worse). I guess we take a risk that the GC becomes ''slightly'' slower with that patch. But if we do apply it we can safely apply the patch that follows. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8742#comment:16 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#8742: Reuse scavenge_small_bitmap -------------------------------------+------------------------------------ Reporter: Tarrasch | Owner: Tarrasch Type: task | Status: patch Priority: lowest | Milestone: 7.10.1 Component: Runtime System | Version: 7.6.3 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Unknown/Multiple Type of failure: None/Unknown | Difficulty: Unknown Test Case: | Blocked By: Blocking: | Related Tickets: -------------------------------------+------------------------------------ Comment (by Simon Marlow ): In [changeset:"05fcc3331622995015ccba17ae333b9a0fc9bb77/ghc"]: {{{ #!CommitTicketReference repository="ghc" revision="05fcc3331622995015ccba17ae333b9a0fc9bb77" Rts: Reuse scavenge_small_bitmap (#8742) The function was inlined at two places already. And the function is having the STATIC_INLINE annotation, so the assembly output should. be the same. To convince myself, I did diff the output of the object files before and after the patch and they matched on my 64-bit Ubuntu 13.10 machine, running gcc 4.8.1-10ubuntu9. Also, I had to move scavenge_small_bitmap up a bit since it's not in any .h-file. While I was at it, I also applied the analogous patch for Compact.c. Though I had to write `thread_small_bitmap` instead of just moving it. }}} -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8742#comment:18 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler