Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

Kaleidoscope-Ch4 is not working as expected and is missing documentation #51348

Open Quuxplusone opened 2 years ago

Quuxplusone commented 2 years ago
Bugzilla Link PR52381
Status NEW
Importance P enhancement
Reported by yarl baudig (yarl-baudig@mailoo.org)
Reported on 2021-11-02 10:29:55 -0700
Last modified on 2021-11-03 11:12:07 -0700
Version trunk
Hardware Other Linux
CC dblaikie@gmail.com, lhames@gmail.com, llvm-bugs@lists.llvm.org, shivam98.tkg@gmail.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also
First, according to the tutorial, we should be able to do
    ready> def foo(x) x + 1;
    ready> foo(2);
    Evaluated to 3.000000

    ready> def foo(x) x + 2;
    ready> foo(2);
    Evaluated to 4.000000

but if I try this (I compiled llvm with examples):
    sh-5.1$ ./Kaleidoscope-Ch4
    ready> def foo(x) x + 1;
    ready> Read function definition:define double @foo(double %x) {
    entry:
      %addtmp = fadd double %x, 1.000000e+00
      ret double %addtmp
    }

    ready> def foo(x) x + 2;
    ready> Read function definition:define double @foo(double %x) {
    entry:
      %addtmp = fadd double %x, 2.000000e+00
      ret double %addtmp
    }

    Duplicate definition of symbol 'foo'
    sh-5.1$

Second, this line https://github.com/llvm/llvm-
project/blob/052a2913f5ced0d266e946f1e697815df1d1fcc0/llvm/examples/Kaleidoscope/Chapter4/toy.cpp#L612
in the full code listing is not present on its corresponding snippet and thus,
not explained.

Thank you!
Quuxplusone commented 2 years ago

I think for the first, it is in expected behavior. The tutorial just did not mention, that we should not define the duplicate definition of a symbol in the same session, and code snippets are meant to run separate sessions.

For the second, It seems we have to update the tutorial text to match code snippet, patches are welcome for that as mentioned in the commit: https://github.com/llvm/llvm-project/commit/ad92f16ccc5f87625bfea2823cd79700bea52c54 ([ORC][examples] Update Kaleidoscope and BuildingAJIT tutorial series … …to OrcV2).

If you want to give it try, @lhames might review them on llvm phabricator(https://llvm.org/docs/Phabricator.html#phabricator-reviews, https://reviews.llvm.org/). Otherwise, I can also try it on this weekend.

Quuxplusone commented 2 years ago

Thank you for answering. I am afraid that either I disagree or I did not understand : under https://llvm.org/docs/tutorial/MyFirstLanguageFrontend/LangImpl04.html#id4 the tutorial states "we’re going to go a step further and put every function in its own module. Doing so allows us to exploit a useful property of the KaleidoscopeJIT that will make our environment more REPL-like: Functions can be added to the JIT more than once".

I can't patch anything yet. I am a complete beginner.

Quuxplusone commented 2 years ago
(In reply to yarl baudig from comment #2)
> Thank you for answering. I am afraid that either I disagree or I did not
> understand : under
> https://llvm.org/docs/tutorial/MyFirstLanguageFrontend/LangImpl04.html#id4
> the tutorial states "we’re going to go a step further and put every function
> in its own module. Doing so allows us to exploit a useful property of the
> KaleidoscopeJIT that will make our environment more REPL-like: Functions can
> be added to the JIT more than once".

Oops then I missed the reading, I will check the previous versions if they are
working as intended.

> I can't patch anything yet. I am a complete beginner.

If you are motivated, you are very welcome to contribute, and There is a
tutorial that gives a good idea on how to contribute: "TypoFix Tutorial"
https://llvm.org/docs/MyFirstTypoFix.html.
Quuxplusone commented 2 years ago

Well, thank you. I just started learning what and how llvm is. Actually, I also posted to get explanations about getMainJITDyLib (second issue). I can't explain things I don't know :).

I just realize there is another problem (I quickly read to the end of the tutorial) that chapter 4 promise "The KaleidoscopeJIT class is a simple JIT built specifically for these tutorials, [...] In later chapters we will look at how it works and extend it with new features, but for now we will take it as given". But there is not much (not enough?) explanations in later chapters.

Quuxplusone commented 2 years ago
(In reply to yarl baudig from comment #4)
> Well, thank you. I just started learning what and how llvm is. Actually, I
> also posted to get explanations about getMainJITDyLib (second issue). I
> can't explain things I don't know :).
>

Fair enough, I have passed the ball to the senior member of the project. I can
just confirm that It is working as intended in LLVM-11.0.0 but not in LLVM-
12.0.0