Opened 8 years ago

Closed 15 months ago

#709 closed bug (fixed)

"Fixup too large" error with -fasm on PowerPC

Reported by: simonmar Owned by:
Priority: low Milestone: 6.8.1
Component: Compiler (NCG) Version: 7.7
Keywords: Cc: pho@…
Operating System: Unknown/Multiple Architecture: powerpc
Type of failure: Building GHC failed Difficulty: Moderate (less than a day)
Test Case: Blocked By:
Blocking: Related Tickets:

Description

The native code generator on PowerPC can sometimes generate code that doesn't pass the assembler. The error is "Fixup too large" from the assembler.

Workaround is to use -fvia-C.

Wolfgang Thaller says this: Conditional branches on the PowerPC only have 16 bits for the displacement. I have been reluctant to fix this so far because it means either slowing down all conditional branches or actually checking the size of the generated code before deciding what branch instructions to use, which I'm afraid would add an additional pass to the NCG.

Change History (23)

comment:1 Changed 8 years ago by simonmar

See also #717

comment:2 Changed 7 years ago by igloo

  • Milestone set to 6.8

comment:3 Changed 7 years ago by wolfgang

  • Difficulty changed from Difficult (1 week) to Moderate (1 day)
  • Resolution set to fixed
  • Status changed from new to closed

comment:4 Changed 6 years ago by igloo

  • Milestone changed from 6.8 branch to 6.8.1

comment:5 Changed 6 years ago by simonmar

  • Operating System changed from Multiple to Unknown/Multiple

comment:6 Changed 4 years ago by simonmar

  • Difficulty changed from Moderate (1 day) to Moderate (less than a day)

comment:7 Changed 17 months ago by PHO

  • Cc pho@… added
  • Resolution fixed deleted
  • Status changed from closed to new
  • Type of failure set to Building GHC failed
  • Version changed from 6.4.1 to 7.7

The problem has recurred in HEAD:

"inplace/bin/ghc-stage1" -static -prof  -H32m -O    -package-name time-1.4.0.2 -hide-all-packages -i -ilibraries/time/. -ilibraries/time/dist-install/build -ilibraries/time/dist-install/build/autogen -Ilibraries/time/dist-install/build -Ilibraries/time/dist-install/build/autogen -Ilibraries/time/include   -optP-DLANGUAGE_Rank2Types -optP-DLANGUAGE_DeriveDataTypeable -optP-DLANGUAGE_StandaloneDeriving -optP-include -optPlibraries/time/dist-install/build/autogen/cabal_macros.h -package base-4.7.0.0 -package deepseq-1.3.0.2 -package old-locale-1.0.0.5  -Wall -XHaskell2010 -XRank2Types -XDeriveDataTypeable -XStandaloneDeriving -XCPP -O2  -no-user-package-db -rtsopts      -odir libraries/time/dist-install/build -hidir libraries/time/dist-install/build -stubdir libraries/time/dist-install/build -hisuf p_hi -osuf  p_o -hcsuf p_hc -c libraries/time/./Data/Time/Format.hs -o libraries/time/dist-install/build/Data/Time/Format.p_o

/var/folders/rN/rNbpGijHEzGPi-Ux2JwMgU+++TI/-Tmp-/ghc78514_0/ghc78514_0.s:104514:0:
    Fixup of -33272 too large for field width of 16 bits

/var/folders/rN/rNbpGijHEzGPi-Ux2JwMgU+++TI/-Tmp-/ghc78514_0/ghc78514_0.s:104498:0:
    Fixup of -33636 too large for field width of 16 bits

/var/folders/rN/rNbpGijHEzGPi-Ux2JwMgU+++TI/-Tmp-/ghc78514_0/ghc78514_0.s:94635:0:
    Fixup of 33240 too large for field width of 16 bits

/var/folders/rN/rNbpGijHEzGPi-Ux2JwMgU+++TI/-Tmp-/ghc78514_0/ghc78514_0.s:94632:0:
    Fixup of 33316 too large for field width of 16 bits

/var/folders/rN/rNbpGijHEzGPi-Ux2JwMgU+++TI/-Tmp-/ghc78514_0/ghc78514_0.s:94629:0:
    Fixup of 33392 too large for field width of 16 bits

/var/folders/rN/rNbpGijHEzGPi-Ux2JwMgU+++TI/-Tmp-/ghc78514_0/ghc78514_0.s:94547:0:
    Fixup of 33504 too large for field width of 16 bits

/var/folders/rN/rNbpGijHEzGPi-Ux2JwMgU+++TI/-Tmp-/ghc78514_0/ghc78514_0.s:94486:0:
    Fixup of 33640 too large for field width of 16 bits

I found it was triggered by an extremely large function with lots of info tables. While AsmCodeGen.makeFarBranches tries to transform problematic PPC.Instr.BCC to PPC.Instr.BCCFAR, it underestimates the maximum number of instructions to jump over as it doesn't take account of the presence of info tables.

Here is my patch to work around:

% git fetch http://github.com/phonohawk/ghc.git ticket-709

comment:8 Changed 17 months ago by PHO

  • Status changed from new to patch

comment:9 follow-up: Changed 17 months ago by simonmar

Hmm, this is an unpleasant bit of code :-) hardcoded constants and guesses everywhere.

Could we not use something better than a hardcoded "5" for the info table size? We know the size of the info table for a continuation: sizeof(StgRetInfoTable), and add to that the maximum offset due to alignment.

You could also do better than length blocks: the actual number of info tables is available.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 17 months ago by PHO

Replying to simonmar:

Hmm, this is an unpleasant bit of code :-) hardcoded constants and guesses everywhere.

It is unpleasant indeed. My patch makes a dirty hack even dirtier :(

Could we not use something better than a hardcoded "5" for the info table size? We know the size of the info table for a continuation: sizeof(StgRetInfoTable), and add to that the maximum offset due to alignment.

The size of StgRetInfoTable varies depending on the "way" (e.g. -prof) so we can't simply grab it from the C compiler. I think it's easy to
calculate the size of each tables represented as CmmStatics.

You could also do better than length blocks: the actual number of info tables is available.

Right, but since info tables are scattered around a proc, I couldn't simply replace length blocks with the actual number of tables.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 17 months ago by simonmar

Replying to PHO:

Replying to simonmar:

Could we not use something better than a hardcoded "5" for the info table size? We know the size of the info table for a continuation: sizeof(StgRetInfoTable), and add to that the maximum offset due to alignment.

The size of StgRetInfoTable varies depending on the "way" (e.g. -prof) so we can't simply grab it from the C compiler. I think it's easy to
calculate the size of each tables represented as CmmStatics.

It also varies based on tablesNextToCode and other things, but you only need the upper bound here. I would add an stgRetInfoTableMaxWords :: Int somewhere, maybe CmmInfo.

You could also do better than length blocks: the actual number of info tables is available.

Right, but since info tables are scattered around a proc, I couldn't simply replace length blocks with the actual number of tables.

I don't really understand that. But I don't mind if you want to use length blocks, it's just a bit pessimistic.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 17 months ago by PHO

Replying to simonmar:

It also varies based on tablesNextToCode and other things, but you only need the upper bound here. I would add an stgRetInfoTableMaxWords :: Int somewhere, maybe CmmInfo.

That would be handy though I have no idea how to generate such a constant. Do you intend to hardcode it?

You could also do better than length blocks: the actual number of info tables is available.

Right, but since info tables are scattered around a proc, I couldn't simply replace length blocks with the actual number of tables.

I don't really understand that. But I don't mind if you want to use length blocks, it's just a bit pessimistic.

I found it was my misunderstanding while I was trying to explain why it was wrong to calculate nearLimit by the number of tables. Sorry, you were right.

comment:13 in reply to: ↑ 12 Changed 16 months ago by simonmar

Replying to PHO:

Replying to simonmar:

It also varies based on tablesNextToCode and other things, but you only need the upper bound here. I would add an stgRetInfoTableMaxWords :: Int somewhere, maybe CmmInfo.

That would be handy though I have no idea how to generate such a constant. Do you intend to hardcode it?

No, there are some constants already available from StgCmmLayout, e.g. stdInfoTableSizeB. You may need to add something to give you the size of a return info table though.

comment:14 Changed 16 months ago by PHO

OK I revised my fix to a series of 3 patches. Could you review it?

comment:15 Changed 15 months ago by marlowsd@…

commit 48b958948cc36b3cad95e8661a642b21f120b468

Author: Simon Marlow <marlowsd@gmail.com>
Date:   Wed Jan 23 10:19:25 2013 +0000

    Tidy up: move info-table related stuff to CmmInfo
    
    Prep for #709

 compiler/cmm/CmmInfo.hs          |  155 +++++++++++++++++++++++++++++++++++++-
 compiler/cmm/CmmLayoutStack.hs   |    2 +-
 compiler/cmm/CmmParse.y          |    1 +
 compiler/codeGen/StgCmmBind.hs   |    1 +
 compiler/codeGen/StgCmmExpr.hs   |    1 +
 compiler/codeGen/StgCmmLayout.hs |  122 +-----------------------------
 compiler/codeGen/StgCmmPrim.hs   |    1 +
 compiler/ghci/DebuggerUtils.hs   |    2 +-
 8 files changed, 161 insertions(+), 124 deletions(-)

comment:16 Changed 15 months ago by simonmar

Ok, I redid your first patch (see above), your other changes are fine except don't forget to take into account the effect of alignment, which could add a further (word_size - 1) bytes to the size of the info table.

comment:17 Changed 15 months ago by PHO

Thanks for tidying up, but I think we don't have to worry about section alignment because PPC is an architecture where every instruction has a fixed length of word_size (unlike e.g. x86), and the size of info table is always a multiple of word_size.

comment:18 Changed 15 months ago by simonmar

Ok yes, thanks for pointing that out. Would you mind rebasing your patches and I'll get them committed?

comment:19 Changed 15 months ago by PHO

Sure.

comment:20 Changed 15 months ago by PHO

I rebased my second and third patch:

% git fetch http://github.com/phonohawk/ghc.git ticket-709

comment:21 Changed 15 months ago by marlowsd@…

commit 8a6e330ac9c2dde11b0641dce69bb809af6a494c

Merge: ca5d15a... 3cedbfb...
Author: Simon Marlow <marlowsd@gmail.com>
Date:   Mon Feb 4 11:19:33 2013 +0000

    Merge commit '3cedbfb49996da2f029b4a84ca39f4d21f309813'
    
    * commit '3cedbfb49996da2f029b4a84ca39f4d21f309813':
      AsmCodeGen.NcgImpl.ncgMakeFarBranches should take account of info tables (#709)
      Move AsmCodeGen.makeFarBranches to PPC.Instr (#709)

 compiler/nativeGen/AsmCodeGen.lhs |   47 ++++---------------------------------
 compiler/nativeGen/PPC/Instr.hs   |   42 ++++++++++++++++++++++++++++++++-
 2 files changed, 46 insertions(+), 43 deletions(-)

comment:22 Changed 15 months ago by pho@…

commit 3cedbfb49996da2f029b4a84ca39f4d21f309813

Author: PHO <pho@cielonegro.org>
Date:   Thu Dec 20 08:13:37 2012 +0900

    AsmCodeGen.NcgImpl.ncgMakeFarBranches should take account of info tables (#709)
    
    We have to reduce the maximum number of instructions to jump over depending on the number of info tables in a proc.

 compiler/nativeGen/AsmCodeGen.lhs |    8 ++++----
 compiler/nativeGen/PPC/Instr.hs   |   16 +++++++++-------
 2 files changed, 13 insertions(+), 11 deletions(-)

comment:23 Changed 15 months ago by simonmar

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

pushed, thanks!

Note: See TracTickets for help on using tickets.