Opened 3 years ago

Closed 2 years ago

#5576 closed bug (fixed)

Fix to #5549 breaks integerConstantFolding

Reported by: daniel.is.fischer Owned by: simonpj
Priority: highest Milestone: 7.4.1
Component: Compiler Version: 7.3
Keywords: Cc:
Operating System: Linux Architecture: Unknown/Multiple
Type of failure: None/Unknown Difficulty:
Test Case: Blocked By:
Blocking: Related Tickets:

Description

The fix to #5549 makes integerConstantFolding break (on x86 and x86_64 linux at least).

=====> integerConstantFolding(normal) 1229 of 3047 [3, 4, 0]
cd ./lib/integer && $MAKE -s --no-print-directory integerConstantFolding    </dev/null >integerConstantFolding.run.stdout 2>integerConstantFolding.run.stderr
Actual stdout output differs from expected:
--- ./lib/integer/integerConstantFolding.stdout	2011-09-26 12:57:40.214838002 +0200
+++ ./lib/integer/integerConstantFolding.run.stdout	2011-10-22 14:00:27.814342001 +0200
@@ -1,3 +1,10 @@
+Unfolded values found
+lvl9_r2gC = __integer 100060
+lvl13_r2gH = __integer 100059
+quotRemInteger didn't constant fold
+quotRemInteger didn't constant fold
+divModInteger didn't constant fold
+divModInteger didn't constant fold
 plusInteger: 200007
 timesInteger: 683234160
 minusIntegerN: -991
*** unexpected failure for integerConstantFolding(normal)

On x86_64, building without changeset:ca380cd19530fad4860e42844e42a473188e0013 makes T5549 fail,

bytes allocated 12228725064 is more than maximum allowed 8000000000
*** unexpected failure for T5549(normal)

On x86 linux, T5549 fails with too good stats
(3362960220 allocated, less than minimum allowed 5000000000 for current HEAD, 3362960124 for ghc-7.3.20110913, HEAD without ca380cd currently building). The patch doesn't make much difference there for the test case, apparently (seems to make a difference on x86/Darwin, though?).

Can we get good constant folding without losing too much sharing?

Change History (5)

comment:1 Changed 3 years ago by daniel.is.fischer

Okay, on 32-bits, HEAD minus ca380cd allocates almost twice as much as with. Turns out the improved treatment of integer literals was from 17th September, so the old HEAD I still had lying around didn't allocate a fresh Integer in the loop. Difference in running times isn't nearly as dramatic as the difference in allocation, about 10% on x86, about 5% on x86_64 (big cheer for GHC's allocator), but it's still a loss.

I'll play a bit with litSize, see if some tweaking there makes both tests happy.

comment:2 Changed 3 years ago by daniel.is.fischer

Nope. divMod and quotRem get constant-folded (and T5549 breaks) iff litSize (LitInteger{}) = 0.
We need something better to get both goodies.

comment:3 Changed 2 years ago by simonmar

  • Milestone set to 7.4.1
  • Owner set to simonpj
  • Priority changed from normal to highest

Simon, could you take a look at this please? validate is currently broken.

comment:4 Changed 2 years ago by simonpj@…

commit a1448ec2f8a50c76ec13bb61cb7bc87e7f7f101c

Author: Simon Peyton Jones <simonpj@microsoft.com>
Date:   Mon Oct 24 18:05:42 2011 +0100

    Make the matching in PrelRules "look through" unfoldings
    
    This is important for Integer literals; fixes Trac #5576

 compiler/coreSyn/CoreSubst.lhs  |   18 +++++++++++++-
 compiler/coreSyn/CoreUnfold.lhs |    2 +-
 compiler/prelude/PrelRules.lhs  |   50 ++++++++++++++++++++++++--------------
 3 files changed, 49 insertions(+), 21 deletions(-)

comment:5 Changed 2 years ago by simonpj

  • Resolution set to fixed
  • Status changed from new to closed

Fixed!

Simon

Note: See TracTickets for help on using tickets.