Opened 8 months ago

Closed 7 months ago

#8250 closed bug (fixed)

cgrun072 (optllvm) failing

Reported by: leroux Owned by:
Priority: normal Milestone:
Component: Compiler (LLVM) Version: 7.6.3
Keywords: Cc: leroux@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Runtime crash Difficulty: Unknown
Test Case: cgrun072 Blocked By:
Blocking: Related Tickets: 7902

Description

  • Platform: OS X 10.8.4 x86_64
  • GHC Version 7.7.20130904 (built with gcc-4.8)

To reproduce this:

$ make test TEST=cgrun072 WAY=optllvm

Expected output (for failure):

=====> cgrun072(optllvm) 172 of 3749 [0, 0, 2]
cd ./codeGen/should_run && '/Users/leroux/Dropbox/src/ghc/ghc-validate/inplace/bin/ghc-stage2' -fforce-recomp -dcore-lint -dcmm-lint -dno-debug-output -no-user-package-db -rtsopts -fno-ghci-history -o cgrun072 cgrun072.hs -O -fllvm   >cgrun072.comp.stderr 2>&1
cd ./codeGen/should_run && ./cgrun072    </dev/null >cgrun072.run.stdout 2>cgrun072.run.stderr
Actual stdout output differs from expected:
--- ./codeGen/should_run/cgrun072.stdout	2013-09-04 02:22:32.000000000 -0500
+++ ./codeGen/should_run/cgrun072.run.stdout	2013-09-07 03:27:09.000000000 -0500
@@ -1,3 +1,6 @@
 OK
-OK
+FAIL
+   Input: 1480294021
+Expected: 2239642456
+  Actual: -2055324840
 OK
*** unexpected failure for cgrun072(optllvm)

The failing test case is test_bSwap32.

Here are some relevant snippets. bswap and cgrun072 were added in #7902.

https://github.com/ghc/testsuite/blob/master/tests/codeGen/should_run/cgrun072.hs:

bswap32 :: Word32 -> Word32
bswap32 (W32# w#) = W32# (byteSwap32# w#)

slowBswap32 :: Word32 -> Word32
slowBswap32 w =
         (w `shiftR` 24)             .|. (w `shiftL` 24)
     .|. ((w `shiftR` 8) .&. 0xff00) .|. ((w .&. 0xff00) `shiftL` 8)

test_bSwap32 = test casesW32 bswap32 slowBswap32

https://github.com/ghc/packages-ghc-prim/blob/master/cbits/bswap.c#L10-L17:

extern StgWord32 hs_bswap32(StgWord32 x);
StgWord32
hs_bswap32(StgWord32 x)
{
  return ((x >> 24) | ((x >> 8) & 0xff00) |
          (x << 24) | ((x & 0xff00) << 8));
}

Here are a few things to look at or try.

Change History (12)

comment:1 Changed 8 months ago by rwbarton

Thanks for catching this, I can reproduce it on Linux/x86_64 also.

There's a similar issue with bswap16 that cgrun072 doesn't catch, because there are no bswap16 test cases with low byte >= 128.

comment:2 Changed 8 months ago by rwbarton

This is a sign-extension issue: the expected output is 2239642456 = 0x857e3b58, which treated as a signed 32-bit integer is 2239642456 - 2^32 = -2055324840. Obviously, show on a Word32 shouldn't be producing output starting with a minus sign!

  • The NCG generates code for MO_BSwap W32 that does the byte swap and clears the high 32 bits of the result. (This isn't directly relevant to this test failure, but provides context for the rest.)
  • The LLVM backend generates LLVM code that does the byte swap and then sign-extends the result to 64 bits, like this:
      %ln2uw = trunc i64 %ln2uv to i32
      %ln2ux = call ccc i32 (i32)* @llvm.bswap.i32( i32 %ln2uw )
      %ln2uy = sext i32 %ln2ux to i64
    
    The reason is that genCallSimpleCast does castVars before and after the call to llvm.bswap.i32, and castVars produces LM_Sext for a widening conversion. That's what's causing the strange test output—the payload of a Word32# isn't supposed to have any of the high 32 bits set.
  • The primop BSwap32Op is documented as
    primop   BSwap32Op   "byteSwap32#"   Monadic   Word# -> Word#
        {Swap bytes in the lower 32 bits of a word. The higher bytes are undefined. }
    
    so both the NCG and LLVM backends are correct, and the test is wrong. It should be doing
    bswap32 :: Word32 -> Word32
    bswap32 (W32# w#) = W32# (narrow32Word# (byteSwap32# w#))
    
    and the same for bswap16.
  • The definitions of byteSwap32 and byteSwap16 in GHC.Word are also wrong for the same reason. They should be
    byteSwap32 :: Word32 -> Word32
    byteSwap32 (W32# w#) = W32# (narrow32Word# (byteSwap32# w#))
    
    This doesn't currently make a difference, though, because base is built (by default?) with the NCG, which happens to produce a zero-extended result.

An alternative approach would be to redefine the BSwap32Op primop to clear the higher bytes of its result. Then the LLVM backend would need to be fixed somehow (looks a little tricky to me) but the cgrun072 test and GHC.Word would be correct as they are.

Last edited 8 months ago by rwbarton (previous) (diff)

comment:3 Changed 8 months ago by rwbarton

A correction to the last bullet point in my previous comment: fixing GHC.Word.byteSwap{16,32} actually *does* matter because a user program built with -O -fllvm will inline those functions at the Haskell level, and then the LLVM backend will generate the wrong code for the byte-swap primop.

import GHC.Word
main = print $ byteSwap32 (0xaabbccdd :: Word32)
-- built with ghc -O -fllvm
-- expected output: 3721182122
-- actual output: -573785174

The cgrun072 test should test the GHC.Word.byteSwap* wrappers, too.

Changed 8 months ago by rwbarton

comment:4 Changed 8 months ago by rwbarton

  • Status changed from new to patch

The first two patches add more tests to cgrun072. The other two patches fix the use of byteSwap16/32# in cgrun072 and in GHC.Word, as described above (treating the description of the byteSwap primops as leaving the higher bytes of the result undefined as correct).

comment:5 Changed 7 months ago by thoughtpolice

Looks good to me - thanks for the investigation Reid. I'll probably squash these into one patch for base and merge them later tonight.

comment:6 Changed 7 months ago by Austin Seipp <austin@…>

In c8cd4dfa480bd259bb889c3e679393ffa426abb3/testsuite:

Fix up cgrun072 a bit (#8250)

This includes:

 * Adding a test for bswap16 with a low byte >= 128
 * Also test the byteSwapN functions from GHC.Word, tested both INLINE
   and not INLINE, so we test both independent parts: the compilation of
   base, and the backend compiling the code *using* base.
 * Fix the usage of byteSwapN# primitives in the test, by masking off
 * the higher bits when storing the results in Word16/Word32.

Thanks to Reid Barton for the investigation.

Authored-by: Reid Barton <rwbarton@gmail.com>
Signed-off-by: Austin Seipp <austin@well-typed.com>

comment:7 Changed 7 months ago by thoughtpolice

Merged, thanks Reid!

comment:8 Changed 7 months ago by thoughtpolice

  • Resolution set to fixed
  • Status changed from patch to closed
Note: See TracTickets for help on using tickets.