LLVM backend miscompilation

This Cmm program produces different result when compiled using the NCG and LLVM backends:

test() {
  return (%zx64(%lobits16(%zx32(%lobits8((%lobits16(%shl((1 :: bits32), (45 :: bits64))) ^ (1 :: bits16)))))));
}

The NCG returns 1 while LLVM returns 0 with -O0 and 0x7ff3a69bc010 with -O1. I believe 1 is correct.

added LLVM backend Phigh Tbug incorrect runtime result labels

changed the description

My manual reduction:

%zx64(%lobits16(%zx32(%lobits8((%lobits16(%shl((1 :: bits32), (45 :: bits64))) ^ (1 :: bits16))))))
                      |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^                  |
                      |                   == 0 :: W32                                          |
                      |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^                 |
                      |         == 0 :: W16                                                    |
                      |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ |
                      |        == 1 :: W16                                                     |
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                       == 1 :: W8

This appears to be similar to #20626 (closed): LLVM defines shifts by amounts larger than the bit size of the shifted value to be undefined.

marked this issue as related to #20626 (closed)

Actually, this is testcase is invalid. Cmm's shl is also undefined in the case of too-large shifts.

Actually, I take it back. primops.txt.pp claims only stipulates that the shift amount must be in [0, word_size-1). However, in this case the "shiftee" is smaller than a word.

I see three ways to deal with this:

1. refine the definition of the Cmm shift operations, requiring that the shift amount be rather in [0, shiftee_size-1], or
2. allow large shifts in C-- and make sure that they are folded to 0 before making it to the backend, or
3. allow large shifts in C-- and let the backend deal with cases like this (namely shift amounts in [shiftee_size, word_size])

I feel like (1) is the most principled approach, but I am a bit worried that this might bite existing users. We can modify base to ensure that the shift operations for the lifted sub-word integer types catch large shifts, but this still leaves existing users of the unlifted types with potential undefined behavior.

but this still leaves existing users of the unlifted types with potential undefined behavior.

1. seems reasonable to me for the Cmm side.

We already give unsafeShiftL in based UB for large shifts: https://hackage.haskell.org/package/base-4.16.0.0/docs/Data-Bits.html#v:unsafeShiftL

closed

reopened

mentioned in commit 9346a5bd

mentioned in commit 25202deb

mentioned in commit ae182376

mentioned in commit 48efa74d

mentioned in commit aadea655

mentioned in commit 49846d04

mentioned in commit ffddf8c1

assigned to @bgamari

mentioned in commit e594ac9a

mentioned in commit eff1e0b6

mentioned in commit 1aa1d574

mentioned in merge request !6934 (closed)

mentioned in commit 4ec707d6

mentioned in commit d955ea82

mentioned in commit 93f52f14

mentioned in commit c2c54fb6

mentioned in commit 35bbc251

mentioned in commit 8459c7b9

mentioned in commit f30469e8

Can this be closed now that !6934 (closed) has been merged?

closed

Indeed this is now fixed.