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

Mistake in generation of Egg expressions #113

Closed
JonathanDLTran opened this issue Sep 30, 2021 · 6 comments
Closed

Mistake in generation of Egg expressions #113

JonathanDLTran opened this issue Sep 30, 2021 · 6 comments

Comments

@JonathanDLTran
Copy link
Collaborator

The output after Egg optimization produces a sequence of Egg expressions nodes that do not correspond to any intended pattern in the conversion back to LLVM IR.

Input:
RecExpr { nodes: [Symbol("no-array-name7"), Symbol("1,"), Get([0, 1]), Add([2, 0]), Symbol("no-array-name8"), Symbol("2,"), Get([4, 5]), Symbol("no-temp-name6"), Mul([6, 7]), Symbol("no-array-name9"), Symbol("2,"), Get([9, 10]), Add([11, 8]), Symbol("no-array-name10"), Symbol("3,"), Get([13, 14]), Symbol("no-array-name11"), Symbol("2,"), Get([16, 17]), Mul([15, 18]), Num(0), Vec([3, 12, 19, 20])] }

Output:
[Symbol("no-array-name7"), Symbol("1,"), Get([0, 1]), Symbol("no-array-name9"), Symbol("2,"), Get([3, 4]), Num(0), Num(0), Vec([2, 5, 6, 7]), Num(1), Symbol("no-array-name8"), Symbol("2,"), Get([10, 11]), Symbol("no-array-name10"), Symbol("3,"), Get([13, 14]), Num(1), Vec([9, 12, 15, 16]), Symbol("no-array-name7"), Symbol("no-temp-name6"), Symbol("no-array-name11"), Symbol("2,"), Get([20, 21]), Num(0), Vec([18, 19, 22, 23]), VecMAC([8, 17, 24])]

Error:
thread '<unnamed>' panicked at '"no-array-name7"', src/lib.rs:398:15
Symbol("no-array-name7") should always be paired with an array offset Symbol Node and a Get Node. In this situation, the Symbol("no-array-name7") should not exist by itself.

@sampsyo
Copy link
Contributor

sampsyo commented Sep 30, 2021

Interesting… maybe I'm just being slow, but I guess I expected that these would be legible as S-expressions instead of this flat symbol list. (Put differently, I'm not exactly sure how to read this RecExpr dump as an expression.)

Is there a chance that this is the output of just formatting a RecExpr directly (i.e., .to_string()), but calling the pretty() method on RecExpr would yield a more legible S-expression?

@JonathanDLTran
Copy link
Collaborator Author

JonathanDLTran commented Oct 2, 2021

I should have started off by printing the S-expressions, and I realized that the mistake arises in how I convert the LLVM IR to Egg expressions, so I have changed the title of the issue to be less misleading.

The new S-expression input is:
(VecMAC (Vec no-temp-name4 (Get no-array-name4 1,) 0 0) (Vec 1 no-temp-name5 (Get no-array-name5 1,) 1) (Vec no-temp-name4 (Get no-array-name3 1,) (Get no-array-name6 3,) 0))
and the new S-expression output is
(VecMAC (Vec (Get no-array-name7 1,) (Get no-array-name9 2,) 0 0) (Vec 1 (Get no-array-name8 2,) (Get no-array-name10 3,) 1) (Vec no-array-name7 no-temp-name6 (Get no-array-name11 2,) 0))

The corrected reason for the problem is that no-array-7 appears by itself as a node in the input S-expression.

@JonathanDLTran JonathanDLTran changed the title Mistake in Egg optimization Mistake in generation of Egg expressions Oct 2, 2021
@sampsyo
Copy link
Contributor

sampsyo commented Oct 3, 2021

Huh, interesting… this might be dumb, but should I be surprised that the symbol no-array-name7 only appears in the output? It seems weird that egg optimization would "invent" a new array instead of just rearranging accesses to existing arrays…

@JonathanDLTran
Copy link
Collaborator Author

This is completely my mistake; I made a copy and paste error, because the correct initial input should be:
(Vec (+ (Get no-array-name7 1,) no-array-name7) (+ (Get no-array-name9 2,) (* (Get no-array-name8 2,) no-temp-name6)) (* (Get no-array-name10 3,) (Get no-array-name11 2,)) 0)

@sampsyo
Copy link
Contributor

sampsyo commented Oct 4, 2021

Got it. I'm still not 100% clear on why the conclusion is that the LLVM->egg translation is wrong... (or did I misunderstand)? Can you point to the subexpression that was generated incorrectly?

@avanhatt
Copy link
Contributor

Now resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants