Opened 2 years ago

Closed 2 years ago

#14140 closed bug (fixed)

Better treatment for dataToTag

Reported by: simonpj Owned by:
Priority: normal Milestone: 8.4.1
Component: Compiler Version: 8.2.1
Keywords: datacon-tags Cc: dfeuer
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Runtime performance bug Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D3934
Wiki Page:

Description

In libraries/Cabal/Cabal/Distribution/System.hs I found

eqOS :: OS -> OS -> Bool
eqOS (OtherOS s1) (OtherOS s2) = s1 == (s2 :: String)
eqOs a b = dataToTag a == dataTag b

(actually it's not called eqOS; it's the derived equality for OS).

By the time this gets to Core it looks something like this

eqOS a b
  = case a of
      OtherOS s1 -> case b of
                       OtherOS s2 -> eqString s1 s2
                       _ -> case dataToTag b of
                              16# -> True
                              r   -> False
      _ -> dataToTag a == dataToTag b

The dataToTag code has been duplicated; and in the OtherOS s1 branch GHC can constant-fold the dataToTag on a to 16#, the tag of OtherOS. But GHC is no so clever for the dataToTag b. We know that b is not OtherOS, so we know its tag is not 16#, so the innermost case is entirely redundant. But we don't spot that.

Change History (14)

comment:1 Changed 2 years ago by bgamari

Keywords: datacon-tags added

comment:2 Changed 2 years ago by dfeuer

For derived instances, might it make sense to write this?

eqOS :: OS -> OS -> Bool
eqOS a b | dataToTag a /= dataToTag b = False
eqOS (OtherOS s1) (OtherOS s2) = s1 == (s2 :: String)
eqOS _ _ = True

Sometimes this will be better; sometimes it will be worse. But I don't think it's likely to do anything silly. Of course, even if we do that, we probably want to try to make the compiler cleverer too.

Last edited 2 years ago by dfeuer (previous) (diff)

comment:3 Changed 2 years ago by dfeuer

Simon, could you point me to the code responsible for the constant folding here? I'd like to see if I can guess how to make it do what you want.

comment:4 Changed 2 years ago by dfeuer

Cc: dfeuer added

comment:6 Changed 2 years ago by simonpj

hysl20 is right. Doing stuff in PrelRules works if the rewrite we want is something like dataToTag True ---> 1.

But for this ticket we don't want that. What we know is negative information: dataToTag x cannot be 16#. This is carried by the OtherCon uunfolding for x.

So the place to implement this ticket (if that is behind your question) is SimplUtils.prepareAlts. We want to say "if the scrutinee is dataToTag x and x has an unfolding of OtherCon [A,B,C], then eliminate any alternatives that match those constructors". Something like this in prepareAlts

imposs_cons = case scrut of
                Var v -> otherCons (idUnfolding v)  -- as now
                App dataToTag (Var v)  -> map (LitCon . conToTag)
                                              (otherCons (idUnfolding v))
                _ -> []

That is,

  • If the scrutinee is dataToTag v
  • And v cannot be [A,C,F]
  • Then we know that dataToTag v cannot be [1#, 3#, 6#]
  • And so we can make a suitable imposs_cons

Does that help?

comment:7 Changed 2 years ago by dfeuer

Yes, that's exactly what I was looking for. I'm trying it now.

comment:8 Changed 2 years ago by dfeuer

Differential Rev(s): Phab:D3932
Status: newpatch

I think I probably did that right...

comment:9 Changed 2 years ago by dfeuer

Argh! I should have checked this against HEAD. It seems this was already fixed, in 193664d42dbceadaa1e4689dfa17ff1cf5a405a0. We now rewrite

case dataToTag b of
  16# -> True
  _   -> False

to

case b of
  OtherOS _ -> True
  _ -> False

which has always been good.

comment:10 Changed 2 years ago by simonpj

Huh, you are right. And that is a better place to do it.

So, why isn't it working?

comment:11 Changed 2 years ago by dfeuer

Differential Rev(s): Phab:D3932Phab:D3934

I think it is working, unless it broke very very recently. I've just changed the linked differential to a test case. When I compiled various versions of your code under GHC HEAD (September 1 edition or so), I got exactly what we want.

comment:12 Changed 2 years ago by simonpj

I think it is working, unless it broke very very recently

You are right. How mysterious. I can't account for why I ever saw the bad code. But indeed it seems fixed. I recompiled libraries/Cabal/Cabal/Distribution/System.hs and it looks fine.

Close when you commit the test case.

comment:13 Changed 2 years ago by David Feuer <David.Feuer@…>

In 0ebc8dc3/ghc:

Add a test for #14140

This issue seems to have been fixed by
193664d42dbceadaa1e4689dfa17ff1cf5a405a0 (Re-engineer caseRules
to add tagToEnum/dataToTag) but I don't think anyone realized
that.

Reviewers: simonpj, austin, bgamari

Reviewed By: simonpj

Subscribers: rwbarton, thomie

GHC Trac Issues: #14140

Differential Revision: https://phabricator.haskell.org/D3934

comment:14 Changed 2 years ago by dfeuer

Milestone: 8.4.1
Resolution: fixed
Status: patchclosed
Type of failure: None/UnknownRuntime performance bug
Note: See TracTickets for help on using tickets.