Opened 4 years ago

Closed 4 years ago

#10870 closed bug (fixed)

PPC.Ppr: Shift by 32 bits is not allowed.

Reported by: nomeata Owned by:
Priority: normal Milestone: 7.10.3
Component: Compiler (CodeGen) Version: 7.10.2
Keywords: Cc: erikd, trommler
Operating System: Unknown/Multiple Architecture: powerpc
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D1322, Phab:1459
Wiki Page:

Description

Hi,

when compiling vector-algorithms on powerpc, I get

[ 7 of 10] Compiling Data.Vector.Algorithms.Tim ( src/Data/Vector/Algorithms/Tim.hs, dist-ghc/build/Data/Vector/Algorithms/Tim.o )
ghc: panic! (the 'impossible' happened)
  (GHC version 7.10.2 for powerpc-unknown-linux):
	PPC.Ppr: Shift by 32 bits is not allowed.

I do not get this error on other 32bit architectures.

The offending code seems to be

minrun :: Int -> Int
minrun n0 = (n0 `unsafeShiftR` extra) + if (lowMask .&. n0) > 0 then 1 else 0
 where
 -- smear the bits down from the most significant bit
 !n1 = n0 .|. unsafeShiftR n0 1
 !n2 = n1 .|. unsafeShiftR n1 2
 !n3 = n2 .|. unsafeShiftR n2 4
 !n4 = n3 .|. unsafeShiftR n3 8
 !n5 = n4 .|. unsafeShiftR n4 16
 !n6 = n5 .|. unsafeShiftR n5 32

The call to panic was introduced by Erik in the fix for #5900.

Is the code at fault, or the compiler?

Attachments (2)

0001-PPC-Fix-right-shift-by-32-bits-10870.patch (1.9 KB) - added by erikd 4 years ago.
Patch for 7.10 branch
0001-PPC-nativeGen-fix-shift-right-arithmetic-31-bit.patch (1.1 KB) - added by trommler 4 years ago.
...and for arithmetic right shifts

Download all attachments as: .zip

Change History (24)

comment:1 Changed 4 years ago by erikd

Maybe adding the panic was the wrong thing to do there. On 32 bit PowerPC, a right shift of 32 bits should just result in zero.

However, the PowerPC codegen now supports 64 bit PowerPC, so we need to be careful.

comment:2 Changed 4 years ago by trommler

Cc: trommler added

comment:3 Changed 4 years ago by erikd

I'm currently working on the ghc-7.10 branch because the master branch currently does not compile on PowerPC due to #7830.

I have a test program:

import Data.Bits
import Data.Int
import Data.Word

shift32R :: (Bits a, Num a) => a -> a
shift32R x = unsafeShiftR x 32

main :: IO ()
main = do
    print $ map shift32R [ 123456, 0x7fffffff :: Int ]
    print $ map shift32R [ 123456, 0xffffffff :: Word ]
    print $ map shift32R [ 123456678, 123456678123456678 :: Int64 ]
    print $ map shift32R [ 123456678, 123456678123456678 :: Word64 ]

and I have a fix that seems to work (PowerPC output is same as for x86_64 and Arm), but the compiler prints 14 identical instances of the warning:

WARNING: file compiler/simplCore/SimplEnv.hs, line 530 showl_a1O5

comment:4 Changed 4 years ago by erikd

Recompiled from scratch and the weird warning was gone.

comment:5 Changed 4 years ago by erikd

Differential Rev(s): Phab:D1322

comment:6 Changed 4 years ago by Erik de Castro Lopo <erikd@…>

In 4bd58c1/ghc:

PPC: Fix right shift by 32 bits #10870

Summary: Test included.

Test Plan: Run test T10870.hs on X86/X86_64/Arm/Arm64 etc

Reviewers: bgamari, nomeata, austin

Subscribers: thomie

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

GHC Trac Issues: #10870

comment:7 Changed 4 years ago by erikd

Status: newmerge

comment:8 Changed 4 years ago by erikd

Milestone: 7.10.3

Changed 4 years ago by erikd

Patch for 7.10 branch

comment:9 Changed 4 years ago by erikd

The commit in the master branch doesn't apply to the 7.10 branch because of all the powerpc64el work in master. I have therefore attached a 7.10 specific patch.

comment:10 in reply to:  9 Changed 4 years ago by trommler

Replying to erikd:

The commit in the master branch doesn't apply to the 7.10 branch because of all the powerpc64el work in master.

Speaking of 64-bit: I suppose I should implement shifts by 64 bit in a similar fashion for 64-bit PowerPC. I'll create a new ticket for that.

comment:11 Changed 4 years ago by bgamari

Resolution: fixed
Status: mergeclosed

comment:12 Changed 4 years ago by nomeata

Resolution: fixed
Status: closednew

Sorry to be the bringer of bad news, but this seems to be still occurring in 7.10.3-rc1:

https://buildd.debian.org/status/fetch.php?pkg=haskell-vector-algorithms&arch=powerpc&ver=0.7.0.1-2&stamp=1446804253

comment:13 in reply to:  12 Changed 4 years ago by trommler

Status: newinfoneeded

Replying to nomeata:

Sorry to be the bringer of bad news, but this seems to be still occurring in 7.10.3-rc1:

https://buildd.debian.org/status/fetch.php?pkg=haskell-vector-algorithms&arch=powerpc&ver=0.7.0.1-2&stamp=1446804253

Can you try the above patch (0001-PPC-nativeGen-fix-shift-right-arithmetic-31-bit.patch)?

I could only eye-ball the code (no 32 bit installation here), but I am pretty sure this is the only case left that triggers the panic you are seeing.

Changed 4 years ago by trommler

...and for arithmetic right shifts

comment:14 Changed 4 years ago by nomeata

Thanks! I almost kicked off a full ghc package upload to Debian, but I better wait until you have reached the final version of your patch. (Interesting that trac does not display your updates to the patch, this might be very confusion when people talk about different instances of a patch.)

comment:15 in reply to:  14 ; Changed 4 years ago by trommler

Replying to nomeata:

Thanks! I almost kicked off a full ghc package upload to Debian, but I better wait until you have reached the final version of your patch. (Interesting that trac does not display your updates to the patch, this might be very confusion when people talk about different instances of a patch.)

Sorry about that! I forgot to refresh the patch and uploaded an old version that did not even compile three times. I used the replace attachment option for each upload.

comment:16 in reply to:  15 Changed 4 years ago by trommler

Status: infoneededmerge

Replying to trommler:

Replying to nomeata:

Thanks! I almost kicked off a full ghc package upload to Debian, but I better wait until you have reached the final version of your patch. (Interesting that trac does not display your updates to the patch, this might be very confusion when people talk about different instances of a patch.)

Sorry about that! I forgot to refresh the patch and uploaded an old version that did not even compile three times. I used the replace attachment option for each upload.

The patch is good. I verified on openSUSE build service.

I looked at HEAD again and the code there is not correct for negative numbers. I will post a fix for HEAD on Phab shortly (I am still validating).

Meanwhile 0001-PPC-nativeGen-fix-shift-right-arithmetic-31-bit.patch can be merged into ghc-7.10. Once merged, please reopen, set milestone to 8.0.1 and assign to me (@trommler). Thanks!

comment:17 Changed 4 years ago by nomeata

Yay, vector-algorithms built now on powerpc!

comment:18 in reply to:  17 Changed 4 years ago by trommler

Replying to nomeata:

Yay, vector-algorithms built now on powerpc!

Excellent, I built on openSUSE build service, too.

vector-algorithms builds fine but the testsuite is stuck forever on x86, x86_64, and powerpc. FWIW: powerpc64[le], is also stuck, but my compiler is patched with a back port of the NCG from 8.0.

Do you see that, too?

comment:19 Changed 4 years ago by nomeata

No, passes everywhere. But given the large output of the test suite, this sounds like https://github.com/haskell/cabal/issues/2398. Try to add the patch from https://github.com/haskell/cabal/pull/2913 and add --show-details=direct.

comment:20 Changed 4 years ago by trommler

Differential Rev(s): Phab:D1322Phab:D1322, Phab:1459

I uploaded the fix for HEAD to Phabricator. Thanks to @nomeata for testing and also for the pointers in comment:19!

To sum up:

  1. Phab:1459 has the patch for HEAD
  2. 0001-PPC-nativeGen-fix-shift-right-arithmetic-31-bit.patch attached to this ticket has the patch for ghc-7.10

BTW: Is there a way to post a patch to a branch on Phabricator?

comment:21 Changed 4 years ago by Ben Gamari <ben@…>

In fb0d512/ghc:

nativeGen.PPC: Fix shift arith. right > 31 bits

Arithmetic right shifts of more than 31 bits set all bits to
the sign bit on PowerPC. iThe assembler does not allow shift
amounts larger than 32 so do an arithemetic right shift of 31
bit instead.

Fixes #10870

Test Plan: validate (especially on powerpc)

Reviewers: austin, erikd, bgamari

Reviewed By: bgamari

Subscribers: thomie

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

GHC Trac Issues: #10870

comment:22 Changed 4 years ago by bgamari

Resolution: fixed
Status: mergeclosed
Note: See TracTickets for help on using tickets.