Use isTrue# around primitive comparisons in integer-gmp
ClosedPublic

Authored by rwbarton on Jul 23 2015, 10:52 PM.

Details

Summary

The form

case na# ==# nb# of
  0# -> ...
  _  -> ...

sometimes generates convoluted assembly, see Trac #10676.
timesInt2Integer was the most spectacular offender, especially as
it is a rather cheap function overall (no calls to gmp).

I checked a few instances and some of the old generated assembly
was fine already, but I changed them all for consistency. The new
form is also more consistent with use of these primops in general.

Test Plan

validate

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
rwbarton updated this revision to Diff 3653.Jul 23 2015, 10:52 PM
rwbarton retitled this revision from to Use isTrue# around primitive comparisons in integer-gmp.
rwbarton updated this object.
rwbarton edited the test plan for this revision. (Show Details)
rwbarton added a reviewer: hvr.
hvr accepted this revision.Jul 24 2015, 3:48 PM
hvr edited edge metadata.

LGTM

I assume you've tested this with the "quickest" build-type as well? (there was some brittleness due to pattern-match failures not being optimised away with low optimisation... and in integer-gmp we can't have pattern-match failures)

libraries/integer-gmp/src/GHC/Integer/Type.hs
1853–1856

Btw, if we've got the luxury of Bool anyway, we can just as well use if/then/else, or in this case

isNorm# 
  | isTrue# (szq# ># 1#)   = (indexWordArray# ba# (szq# -# 1#)) `neWord#` 0##
  | True                   = 1#

I only used case because I assumed going through Bools would be overhead... which is kinda ironic, given Trac #10676

This revision is now accepted and ready to land.Jul 24 2015, 3:48 PM
rwbarton updated this revision to Diff 3662.Jul 24 2015, 8:33 PM
rwbarton edited edge metadata.

Use guards in isNorm#

rwbarton marked an inline comment as done.EditedJul 24 2015, 8:34 PM

I didn't realize that was a potential issue, but I have tested with quickest now. I guess even an unoptimizing GHC knows about the law of the excluded middle :)

Of course, could also change all the True to _ if it should be necessary.

libraries/integer-gmp/src/GHC/Integer/Type.hs
1853–1856

Yes, this is nicer and it's the style used in the rest of the file.

hvr added inline comments.Jul 25 2015, 1:32 AM
libraries/integer-gmp/src/GHC/Integer/Type.hs
1853–1856

ok... and what about the other case (_ :: Bool) of { False -> ... ; True -> } above this chunk ? :-)

rwbarton marked an inline comment as done.Jul 25 2015, 9:58 PM
rwbarton added inline comments.
libraries/integer-gmp/src/GHC/Integer/Type.hs
1853–1856

You mean all the other hunks of the diff? They're not in positions that lend themselves to guards (otherwise I guess they would have been guards already), and I wasn't inclined to rearrange as much code as converting to if/then/else would require.

This revision was automatically updated to reflect the committed changes.