SourceNotes are not applied to all identifiers

Compiling

module Foo where

foo x = x + 1

bar y = foo y

with -g produces

$ ghc -g -ddump-ticked test.hs
[1 of 1] Compiling Foo              ( test.hs, test.o )
AbsBinds [a_avC] [$dNum_avD]
  {Exports: [foo <= foo_amB
               <>]
   Exported types: foo :: forall a_avC. Num a_avC => a_avC -> a_avC
                   [LclId, Str=DmdType]
   Binds: -- ticks = [src<test.hs:3:1-13>]
          foo_amB x_alA
            = src<test.hs:3:9-13> (+)
                                    src<test.hs:3:9> x_alA src<test.hs:3:13> 1}
AbsBinds [a_avV] [$dNum_avW]
  {Exports: [bar <= bar_avN
               <>]
   Exported types: bar :: forall a_avV. Num a_avV => a_avV -> a_avV
                   [LclId, Str=DmdType]
   Binds: -- ticks = [src<test.hs:5:1-13>]
          bar_avN y_amz = src<test.hs:5:9-13> foo (src<test.hs:5:13> y_amz)}

Note that neither the occurrence of (+) in foo, nor the occurrence of foo in bar have their own Ticks. Instead they are only covered by the Tick for the entire application x + 1 (resp. foo y).

I'm trying to use the new SourceNote infrastructure to map CoreExprs back to their original source location, but unfortunately I need these locations for each identifier in the source.

Would it be reasonable to add a SourceNote to each occurrence of an identifier?

Trac metadata

Trac field	Value
Version	7.10.1
Type	Bug
TypeOfFailure	OtherFailure
Priority	normal
Resolution	Unresolved
Component	Compiler
Test case
Differential revisions
BlockedBy
Related
Blocking
CC
Operating system
Architecture

changed milestone to %7.10.2

changed weight to 5

added Tbug Trac import labels

This is to do with the HPC program coverage stuff (Andy Gill), right? Or is it DWARF stuff (Peter Wortman)?

Embarrassingly I cannot see documentation in the manual for -g or -ddump-ticked; so that should be fixed too.

-g is the DWARF stuff, but judging by the Coverage module, the DWARF and HPC flags use the same strategy for ticking expressions (TickForCoverage).

I didn't actually change how TickForCoverage behaves, so these design decisions actually go back to HPC. Changing it would be quite easy - probably just replacing a few addTickLHsExprNever in addTickHsExpr by addTickLHsExpr.

On the other hand I'm not quite sure we want that. The original reason was probably that it leads to rather silly stacks of ticks on functions that are applied to many arguments. Plus both source notes and HPC ticks would instantly float upwards, which means that we are not actually gaining any information. Source notes get merged eventually, but for HPC it certainly would make a performance difference.

Do you positively need every single expression annotated?

I don't think we need every expression annotated, but we do need every identifier annotated. This is in the context of LiquidHaskell, where we infer refinement types for Haskell functions. One output mode we have (which is invaluable in my experience) is an HTML version of the Haskell module where each identifier is annotated with a popup containing its refined type. To create this annotated HTML document, we do need precise source locations for each occurrence of an identifier (and lambda expressions).

I doubt the performance overhead would be that bad if we restrict the extra ticks to identifiers and lambdas, but if it turns out to be a problem I'd also be perfectly happy with a separate TickEverything strategy that we can select.

This tick stuff is clearly interesting and useful, but it is serving several masters, which are expressed by CoreSyn.Tickish:

• HpcTick: HPC code coverage
• ProfNote: cost-centre profiling
• Breakpoint: breakpoint debugging
• SourceNote: do not get in the way of optimisation, but help with debugging via DWARF.

They differ in the extent to which they interfere with optimisation. You probably want either SourceNote or HpcTick here; but I'm not sure which. It depends on the semantics you expect during optimisation. I suspect you want SourceNote.

I hope you don't need a fifth one!

Simon

If I understand correctly, this is primarily about inferring type information about the original program from a (ticked) Haskell syntax dump. So it doesn't matter what later stages of GHC do with the ticks, as long as they appear in that particular dump.

And it's quite tempting to do, because we are 99% there and the change would be rather trivial. Main "problem" is that there's simply no good reason in the context of the compiler to behave like this. If we change the default behaviour, it would certainly need a guarding comment that says why this special case exists.

I think I'll make some kind of patch this week, not quite sure what it will look like though.

changed milestone to %7.10.3

Moving to 7.10.3 milestone - if you think this is an error (or a showstopping bug), please remilestone it to 7.10.2 and let us know why.

scpmw, any updates here?

Attached file tick-vars.patch (download).

Proposed patch

Patch attached, but not too happy with it. Making it a dump patch is probably the least offensive option - at least it shows the right intent. Yet it is awkward both in terms of overriding the "tick never" as well as in terms of actually changing the Haskell syntax tree based on a dump flag.

Does this at least help you, gridaphobe?

Thanks scpmw, this looks promising. I'll build LiquidHaskell against your patch and make sure we get the extra Ticks we need.

For our purposes, hiding this behind a flag is fine, but calling it a dump flag seems a bit off to me. As your comment says, the flag is modifying the generated Core, wouldn't a name like -ftick-everything make more sense?

Hrm, this patch doesn't produce the output I would expect. Consider

module Bar where

bar = 1 + 2

$ ghc -ddump-tick-vars -ddump-ticked -g bar.hs
[1 of 1] Compiling Bar              ( bar.hs, bar.o )
AbsBinds [] []
  {Exports: [bar <= bar_amB
               <>]
   Exported types: bar :: Integer
                   [LclId, Str=DmdType]
   Binds: -- ticks = [src<bar.hs:3:1-11>]
          bar_amB
            = src<bar.hs:3:7-11> (+) src<bar.hs:3:7> 1 src<bar.hs:3:11> 2}

The tick on the (+) spans the whole application, which is not precise enough for us.

changed milestone to %8.0.1

This won't be fixed for 7.10.3 as there is no fix yet.

Right. Is this still current? If I remember correctly I tried to track down why my patch doesn't do the right thing here, but got lost somewhere. This is a really easy change, we should resolve it one way or the other.

GHC HEAD as of bbad4f6b (the latest commit I have built locally) still only produces a tick for the entire application expression (missing the (+) occurrence), so I suspect it's still an issue.

It would be really nice to fix this in time for 8.0.

Attached file tick-everything.patch (download).

A variant of scpmw's patch

I've added a slight variant of scpmw's patch that adds ticks everywhere with a new -ftick-everything flag.

It seems that the HsVar pattern was too restrictive. I'm not sure why.. But I don't see any harm in being more generous with Ticks given that this will be hidden behind a flag.

Trac metadata

Trac field	Value
Differential revisions	- → D1565

The diff is blocked on changes so I'm going punt this out of patch state.

changed milestone to %8.2.1

This bug won't be fixed in 8.0.1; bumping to 8.2.

gridaphobe, it seems I must have overlooked your patch when I left ticket:10506#comment:110533. Would it still be helpful to merge it? It seems like a fairly unintrusive change.

It turns out the patch I attached doesn't actually work for my purposes (I think I had only tested it with -ddump-ticked instead of building LiquidHaskell against it). There's a brief discussion of why on Phab (https://phabricator.haskell.org/D1565#inline-13012).

I can work around this by moving simpleOptPgm to a first Core2Core pass, thus exposing the fully unoptimized Core to API users and Plugins. But that's a much more invasive change, and could break existing API users and Plugins.

changed milestone to %8.4.1

Alright, then I'm going to bump this off to 8.4 since the hour is late and there is no obvious solution in sight.

mentioned in issue #14476 (closed)

changed milestone to %8.6.1

This ticket won't be resolved in 8.4; remilestoning for 8.6. Do holler if you are affected by this or would otherwise like to work on it.

gridaphobe, what ended up happening with this?

added Pnormal label