Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Examples of incorrect output from pretty printer #470

Open
tmcdonell opened this issue Sep 25, 2020 · 11 comments
Open

Examples of incorrect output from pretty printer #470

tmcdonell opened this issue Sep 25, 2020 · 11 comments
Labels
bug something isn't working! >_< frontend sharing recovery, fusion, optimisation

Comments

@tmcdonell
Copy link
Member

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 tmcdonell added frontend sharing recovery, fusion, optimisation bug something isn't working! >_< labels Sep 25, 2020
@tmcdonell tmcdonell changed the title Examples of incorrect output from pretty printer: Examples of incorrect output from pretty printer Sep 25, 2020
@tmcdonell
Copy link
Member Author

tmcdonell commented Sep 25, 2020

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.

@tomsmeding
Copy link
Member

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?

@tmcdonell
Copy link
Member Author

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

@tomsmeding
Copy link
Member

tomsmeding commented Sep 25, 2020

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
Copy link
Member

tomsmeding commented Sep 30, 2020

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
Copy link
Member

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.

@ivogabe
Copy link
Contributor

ivogabe commented Oct 13, 2020

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

@ivogabe
Copy link
Contributor

ivogabe commented Oct 13, 2020

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.

@tomsmeding
Copy link
Member

tomsmeding commented Oct 15, 2020

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 :)

This was referenced Oct 23, 2020
@tomsmeding
Copy link
Member

tomsmeding commented Nov 2, 2020

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?)

@tmcdonell
Copy link
Member Author

  • 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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something isn't working! >_< frontend sharing recovery, fusion, optimisation
Projects
None yet
Development

No branches or pull requests

3 participants