Opened 2 years ago

Closed 2 years 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 Test Case: cgrun072
Blocked By: Blocking:
Related Tickets: 7902 Differential Rev(s):


  • 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 > 2>
Actual stdout output differs from expected:
--- ./codeGen/should_run/cgrun072.stdout	2013-09-04 02:22:32.000000000 -0500
+++ ./codeGen/should_run/	2013-09-07 03:27:09.000000000 -0500
@@ -1,3 +1,6 @@
+   Input: 1480294021
+Expected: 2239642456
+  Actual: -2055324840
*** unexpected failure for cgrun072(optllvm)

The failing test case is test_bSwap32.

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

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

extern StgWord32 hs_bswap32(StgWord32 x);
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 2 years 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 2 years 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 2 years ago by rwbarton (previous) (diff)

comment:3 Changed 2 years 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 2 years ago by rwbarton

comment:4 Changed 2 years 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 2 years 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 2 years 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 <[email protected]>
Signed-off-by: Austin Seipp <[email protected]>

comment:7 Changed 2 years ago by thoughtpolice

Merged, thanks Reid!

comment:8 Changed 2 years ago by thoughtpolice

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