Opened 15 months ago

Last modified 5 days ago

#14251 new bug

LLVM Code Gen messes up registers

Reported by: angerman Owned by:
Priority: highest Milestone: 8.8.1
Component: Compiler (LLVM) Version: 8.3
Keywords: Cc: bgamari, simonmar, carter, juhpetersen
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Incorrect result at runtime Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D5190
Wiki Page:

Description

Due to the way the LLVM Code Gen generates Function Singnatures, it is possible to end up mixed up registers.

A slightly adapted T8064

{-# LANGUAGE MagicHash, BangPatterns #-}
module Main where

import GHC.Exts

{-# NOINLINE f #-}
f :: (Int# -> Float# -> Double# -> Float# -> Double# -> String) -> String
f g = g 3# 4.0# 5.0## 6.0# 6.9## ++ " World!"

{-# NOINLINE p #-}
p :: Int# -> Float# -> Double# -> Float# -> Double# -> String
p i j k l m = "Hello"

{-# NOINLINE q #-}
q :: Int# -> Int# -> Float# -> Double# -> Float# -> Double# -> String
q _ i j k l m = "Hello " ++ show (F# l) ++ " " ++ show (D# m)

{-# NOINLINE r #-}
r :: Int# -> Float# -> Double# -> Float# -> Double# -> String
r i = let !(I# z) = length [I# 1# .. I# i] in \j k l m -> p z j k l m
  -- ghc won't eta-expand around the length, because it has unknown cost

main = do
  putStrLn (f p)    -- fast call
  putStrLn (f r)    -- slow call: function but wrong arity
  let g = last [q 1#]
  putStrLn (f g)    -- slow call: thunk

will produce the following results:

../inplace/bin/ghc-stage1 -fllvm -fforce-recomp T6084.hs -O2 -o T6084-llvm && ./T6084-llvm
[1 of 1] Compiling Main ( T6084.hs, T6084.o )
Linking T6084-llvm ...
Hello World!
Hello World!
Hello 4.0 5.0 World!

../inplace/bin/ghc-stage1 -fasm -fforce-recomp T6084.hs -O2 -o T6084-asm && ./T6084-asm
[1 of 1] Compiling Main ( T6084.hs, T6084.o )
Linking T6084-asm ...
Hello World!
Hello World!
Hello 6.0 6.9 World!

The underlying reason is that (at least for X86_64) the Float and Double registers alternate. The llvm code gen creates function signatures based on the live registers (instead of all).

For q only the last Float and Double register are live. However when calling q we pass f1: Float -> d1: Double -> f2: Float -> d2: Double. f2 and d2 are silently ignored, and in the function body, we now have f2 <- f1 and d2 <- d1.

Change History (29)

comment:1 Changed 15 months ago by angerman

Reasoing for putStrLn (f g)

  putStrLn (f g)
= putStrLn (f (last [q 1#]))
= putStrLn (f (q 1#))
= putStrLn ((q 1#) 3# 4.0# 5.0## 6.0# 6.9## ++ " World!")
= putStrLn (q 1# 3# 4.0# 5.0## 6.0# 6.9## ++ " World!")
= putStrLn ("Hello " ++ show (F# 6.0#) ++ " " ++ show (D# 6.9##) ++ " World!")
= putStrLn ("Hello 6.0 6.9 World!")

comment:2 Changed 15 months ago by kavon

I'll take a look at this later today. I fixed a bug almost exactly the same as this problem in my LLVM backend. This fix should not require a lot of changes.

comment:3 Changed 9 months ago by bgamari

What happened to this? It sounds like this bug is still at large.

comment:4 Changed 7 months ago by mbw

Does this imply that using the llvm backend is currently a no-no?

comment:5 Changed 7 months ago by bgamari

It sounds like using it would be a bit risky. Kavon, did anything ever happen on this front?

comment:6 Changed 7 months ago by carter

so anyone using unboxed floats/doubles targetting llvm best be careful of argument order for unlifted double# and float# till this gets fixed?

this is eerily similar to some of the previous float/double register bugs we've seen in ghc, like the 7.8-7.10 series bug and the more recent windows64 abi one

comment:7 Changed 3 months ago by monoidal

Milestone: 8.6.1

Setting a milestone to increase visibility.

If we won't fix it in 8.6 I think this at least deserves a warning in release notes.

comment:8 Changed 3 months ago by George

Is this a regression?

comment:9 Changed 3 months ago by monoidal

It's present at least in 8.2, 8.4 and HEAD.

comment:10 Changed 3 months ago by George

fwiw, imho, documenting this in the release notes makes sense, holding up the release for what may have been a latent bug for a long time does not. Adding a test would be good.

comment:11 Changed 3 months ago by awson

AFAIUI, the most recent takes on this problem are:

  1. https://phabricator.haskell.org/D4003 by @angerman
  2. https://gist.github.com/kavon/566fc6c21ff51803538884b79dc1d841 by @kavon (referred from the former)

My understanding is that "1" doesn't quite work (generates wrong code for handwritten cmm).

Also my understanding is that "2" should work (haven't tested it), but isn't so much aesthetically pleasant, since all padding arguments are of FloatReg type regardless of which type they are at the call-site.

A year has passed since. Neither these two approaches, no other suggested in the email thread started from https://mail.haskell.org/pipermail/ghc-devs/2017-September/014700.html, were elaborated further.

I wonder then what are chances of e.g. "2" to be upstreamed if properly polished?

comment:12 Changed 3 months ago by Ben Gamari <ben@…>

In ba086ca/ghc:

Add testcase for #14251

comment:13 Changed 3 months ago by bgamari

In my opinion just about anything is better than the status quo. It would be great if someone could look into finishing up (2).

comment:14 Changed 3 months ago by kavon

Owner: set to kavon

I'll polish up solution (2) ASAP. Sorry I missed this!

While it seems ugly to add FloatReg as padding, it we need something to "eat up" a floating point register, since they're assigned left-to-right. We know Float and Double are passed in the same register on x86-64 so it should be fine. I need to look into ARM and other calling conventions to make this correct on other systems.

comment:15 Changed 2 months ago by kavon

Differential Rev(s): D5190
Status: newpatch

comment:16 Changed 2 months ago by kavon

Differential Rev(s): D5190Phab:D5190

comment:17 Changed 2 months ago by bgamari

Milestone: 8.6.18.6.2

Thanks kavon!

comment:18 Changed 2 months ago by Ben Gamari <ben@…>

In adcb5fb4/ghc:

Multiple fixes / improvements for LLVM backend

- Fix for #13904 -- stop "trashing" callee-saved registers, since it is
  not actually doing anything useful.

- Fix for #14251 -- fixes the calling convention for functions passing
  raw SSE-register values by adding padding as needed to get the values
  in the right registers. This problem cropped up when some args were
  unused an dropped from the live list.

- Fixed a typo in 'readnone' attribute

- Added 'lower-expect' pass to level 0 LLVM optimization passes to
  improve block layout in LLVM for stack checks, etc.

Test Plan: `make test WAYS=optllvm` and `make test WAYS=llvm`

Reviewers: bgamari, simonmar, angerman

Reviewed By: angerman

Subscribers: rwbarton, carter

GHC Trac Issues: #13904, #14251

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

comment:19 Changed 2 months ago by bgamari

Status: patchmerge

comment:20 Changed 2 months ago by bgamari

Resolution: fixed
Status: mergeclosed

comment:21 Changed 8 weeks ago by juhpetersen

Cc: juhpetersen added

Was this tested on ARM? (see #15780)

comment:22 Changed 8 weeks ago by bgamari

Owner: kavon deleted
Resolution: fixed
Status: closednew

Sadly no; we currently don't have CI for ARM (although I am currently speaking with people to fix this).

Regardless, Kavon is aware of the issue and will hopefully pop up soon with a patch.

comment:24 Changed 7 weeks ago by kavon

Owner: set to kavon

comment:25 Changed 6 weeks ago by Ben Gamari <ben@…>

In d849554/ghc:

Fix for T14251 on ARM

We now calculate the SSE register padding needed to fix the calling
convention in LLVM in a robust way: grouping them by whether
registers in that class overlap (with the same class overlapping
itself).

My prior patch assumed that no matter the platform, physical
register Fx aliases with Dx, etc, for our calling convention.

This is unfortunately not the case for any platform except x86-64.

Test Plan:
Only know how to test on x86-64, but it should be tested on ARM with:

`make test WAYS=llvm && make test WAYS=optllvm`

Reviewers: bgamari, angerman

Reviewed By: bgamari

Subscribers: rwbarton, carter

GHC Trac Issues: #15780, #14251, #15747

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

comment:26 Changed 6 weeks ago by bgamari

Resolution: fixed
Status: newclosed

comment:27 Changed 6 weeks ago by bgamari

Milestone: 8.6.28.6.3
Owner: kavon deleted
Resolution: fixed
Status: closednew

Sadly I had to back out both comment:25 and comment:18 on ghc-8.6 due to breakage. This will likely need to wait for 8.6.3 at the earliest.

comment:28 Changed 5 weeks ago by bgamari

I am backing out both fixes on master as well until we have a fix.

Kavon, if you could find the time it would be amazing if we could have this for 8.8; it would be a shame if we had to ship yet another release with a broken LLVM backend.

comment:29 Changed 5 days ago by bgamari

Milestone: 8.6.38.8.1
Note: See TracTickets for help on using tickets.