accelerate icon indicating copy to clipboard operation
accelerate copied to clipboard

Examples of incorrect output from pretty printer

Open tmcdonell opened this issue 4 years ago • 11 comments

Description This bug contains some examples where the pretty printer currently generates incorrect or ambiguous results. Please feel free to add new examples as you find them!

Steps to reproduce Load up the example(s) and show them.

tmcdonell avatar Sep 25 '20 14:09 tmcdonell

nested :: Exp (Maybe Bool) -> Exp Int
nested = match \case
  Nothing_     -> 0
  Just_ False_ -> 1
  Just_ True_  -> 2

gives (with a sufficiently narrow terminal to not put it on one line):

\(x0, T1 (x1, ())) ->         -- here
  case x0 of
    0 -> 0
    1 -> case x1 of
           0 -> 1
           1 -> 2

EDIT: This has both T1 (even though it is a pair, so should be T2?) as well as pretty printing as a pair itself (,); it should be one or the other.

tmcdonell avatar Sep 25 '20 14:09 tmcdonell

map (\a -> let b = a * a ; c = b * b in T2 c (T3 a b c)) (use (fromList Z [1 :: Float]))

shows as:

map (\x0 -> let x1 = x0 * x0 x2 = x1 * x1 in T2 x2 (T3 x0 x1 x2)) (use (Scalar Z [1.0]))

which needs some ; between the let bindings. If the let-bindings get sufficiently wide that they are split over multiple lines, all is fine again.

Also @tmcdonell I'm not sure what's going wrong in your example; is it the T1?

tomsmeding avatar Sep 25 '20 15:09 tomsmeding

@tomsmeding Yes, it's the T1 (edited the description)

tmcdonell avatar Sep 25 '20 15:09 tmcdonell

EDIT: please see my next message for a smaller repro

This one I haven't been able to reproduce with fusion-processed programs, only on the so-called "internal AST". Therefore, a ghci session:

$ stack repl
...> :m *Data.Array.Accelerate.Trafo
*Data.Array.Accelerate.Trafo> import qualified Data.Array.Accelerate as A
*Data.Array.Accelerate.Trafo A>

Then:

let singleton x = A.use (A.fromList A.Z [x :: Float])
in Sharing.convertAccWith defaultOptions
       (let A.T2 a1 (A.T2 a2 (A.T2 a3 _)) = A.T2 (singleton 1) (A.T2 (singleton 2) (A.T2 (singleton 3) (A.lift ())))
        in A.zipWith (*) a3 a2)

shows as:

let
  T2 a0 (T2 a1 ()) =
    let
      T2 a0 (T2 a1 (T2 a2 ())) =
        let () = () in T2 (use (Scalar Z [1.0])) T2 (use (Scalar Z [2.0])) T2 (use (Scalar Z [3.0])) ()
    in
    T2 a1 T2 a2 ()      -- here
in
zipWith (\x0 x1 -> x0 * x1) a1 a0

which contains T2 a1 T2 a2 (), which is incorrect. This example is probably reducible, but at least this does show it (it seems to be rare somehow?).

EDIT: The line starting with let () = () also has parenthesis-less nested T2 :)

tomsmeding avatar Sep 25 '20 15:09 tomsmeding

Smaller reproducing example for the above missing-parentheses issue:

T2 (T2 (lift ()) (use (fromList Z [1]))) (use (fromList Z [2])) :: Acc (((), Scalar Float), Scalar Float)

shows as:

T2 T2 () (use (Scalar Z [1.0])) (use (Scalar Z [2.0]))

Not sure why I was unable to find this one before.

tomsmeding avatar Sep 30 '20 12:09 tomsmeding

Can I petition for a mode in the pretty-printer that attempts to output valid Haskell input code? Having to manually translate a zillion occurrences of Tn to In is... time-consuming.

tomsmeding avatar Oct 13 '20 15:10 tomsmeding

I think that will be very hard or impossible, as we only have representation types and thus cannot distinguish tuples and shapes.

ivogabe avatar Oct 13 '20 19:10 ivogabe

A related request, can we have globally unique variable names? That'll make it easier to reason about programs. Currently variables can occur in the left hand side of a let-binding and in its binding, as they have different scopes.

By having a separate counter in the pretty printer, to denote the next fresh name, we could have globally unique names.

ivogabe avatar Oct 13 '20 19:10 ivogabe

Another case where putting multiple things on a single line produces ambiguity: (EDIT: actually, more likely to be a precedence issue somewhere)

let a = use (fromList (Z :. (1::Int)) [1.0::Float]) in reshape (I1 (1 - (let I1 x = shape a in x) - (let I1 x = shape a in x))) a

shows as:

let a0 = use (Vector (Z :. 1) [1.0])
in
reshape (1 - let T1 x0 = shape a0 in x0 - let T1 x0 = shape a0 in x0) a0

where the 1 - let A = B in C - let D = E in F part should be 1 - (let A = B in C) - let D = E in F, because otherwise the body of the first let would also contain the second let, which makes the -'s associate incorrectly.

This bit me when trying to feed pretty-printed output back into Accelerate :)

tomsmeding avatar Oct 15 '20 10:10 tomsmeding

Pretty-printing a single-element tuple is inconsistent between left-hand sides and right-hand sides. This is due to the special cases in prettyAtuple and prettyTuple which are not treated specially in prettyLhs.

This results in the situation where the following program:

map (\(I1 x) -> I1 x) (map (\x -> I1 x) (use (fromList (Z :. (1 :: Int)) [1::Int])))

is shown as:

let
  a0 = use (Vector (Z :. 1) [1])
  a1 = map (\x0 -> x0) a0
in
map (\(T1 x0) -> x0) a1

which is type-incorrect as shown, except if you know that both x0 expressions are really T1 x0 in the AST.

I petition to change the case for expressions to explicitly write T1 as well (i.e. to just remove the linked lines in pretty{At,T}uple). An alternative would be to introduce the special case for left-hand sides as well, but I think less ambiguity is better (and currently for expressions and array programs, a is ambiguous between the forms a and ((), a) in the AST).

(EDIT: on an unrelated note, why are those maps not fused? Avoiding to fuse maps that don't do work striking again?)

tomsmeding avatar Nov 02 '20 11:11 tomsmeding

  • I think you are correct in that removing ambiguity is probably better, so let's remove the special cases in the linked lines.
  • yes that should just be manipulating the struct-of-array representation so is silently avoiding fusion (and hoping that the backend does the intended thing)

tmcdonell avatar Nov 04 '20 13:11 tmcdonell