[llvm-dev] Kaleidoscope tutorial: comments, corrections and Windows support

[Moritz Kroll via llvm-dev]

Hi,

I'm currently working my way through the tutorial with LLVM 3.9.1 on 
Windows (finished chapter 4) and stumbled over a few things which could 
be improved:

  - "LLVMContext" does not exist as a variable -> "TheContext"
    - Chapter 3: 5 times
    - Chapter 4: 1 time
    - Chapter 5: 4 times
    - Chapter 6: 2 times
    - Chapter 7: 2 times

3.4. Function Code Generation

  - Wouldn't it make sense to have a
    "Type *DoubleTy = Type::getDoubleTy(TheContext);"?
  - "Function *TheFunction = TheModule->getFunction(Proto->getName());"
    The new getName method is unexpected, the tutorial user has to lookup
    and copy the definition from the full listing.

3.5. Driver Changes and Closing Thoughts

  - "When you quit the current demo"
    -> "When you quit the current demo (by sending an EOF
    via CTRL+D on Linux or CTRL+Z and ENTER on Windows)"
    (Windows users normally don't send EOF characters so they
    usually don't know this shortcut)

4.3. LLVM Optimization Passes

  - Code for InitializeModuleAndPassManager:
    - Remove "Context LLVMContext;"
    - "TheJIT" comes out of nowhere and thus is not defined in the code
      of the tutorial user. Setting the data layout could be added in
      4.4, as it is at least not required for showing the improved body
      of "test"
    - "createBasicAliasAnalysisPass" does not exist anymore (llvm-3.9.1).
      Also requires adaption of the text

4.4. Adding a JIT Compiler

  - Code for main:
    - "TheJIT = llvm::make_unique<KaleidoscopeJIT>();" will cause a crash
      due to an invalid read in "Datalayout::operator=" as
      "EngineBuilder().selectTarget()" returns a nullptr when
      "InitializeNativeTarget" has not been called yet.
      Other problems occur if the other InitializeNative* functions are
      not called. So you should add them here.
    - The (silently added) "TheModule->print" call from chapter 3
      is missing

  - "The KaleidoscopeJIT class is a simple JIT built specifically for
     these tutorials.":
    It would be nice to mention, where to find the include file:
    llvm-src/examples/Kaleidoscope/include/KaleidoscopeJIT.h

  - "auto ExprSymbol = TheJIT->findSymbol("__anon_expr");":
    On Windows, this will always fail as this function will search for
    "ExportedSymbolsOnly" and the exported flag seems to be implemented
    incorrectly for COFF in LLVM (see bug report from Russell Wallace:
    http://lists.llvm.org/pipermail/llvm-dev/2016-April/097964.html ).
    Applied to the current version of Kaleidoscope, he suggested to use
    "CompileLayer.findSymbolIn(H, Name, false)" in
    "KaleidoscopeJIT::findMangledSymbol" to also allow non-exported
    symbols.
    I fixed it by changing "COFFObjectFile::getSymbolFlags" in
    llvm-src/lib/Object/COFFObjectFile.cpp
    to additionally OR SymbolRef::SF_Exported to Result:

> uint32_t COFFObjectFile::getSymbolFlags(DataRefImpl Ref) const {
>   COFFSymbolRef Symb = getCOFFSymbol(Ref);
>   uint32_t Result = SymbolRef::SF_None;
>
>   if (Symb.isExternal() || Symb.isWeakExternal())
>     Result |= SymbolRef::SF_Global | SymbolRef::SF_Exported;

    Works and sounds reasonable to me, but maybe you can prove me wrong.

  - "ready> sin(1.0);": On Windows, this fails with "LLVM ERROR: Program
    used external function '_sin' which could not be resolved!" because
    "DynamicLibrary::SearchForAddressOfSymbol" iterates over all loaded
    libraries and uses GetProcAddress to try getting the address.
    But for the old "msvcrt.dll" as well as the newer "ucrtbased.dll",
    the math functions are exported without a leading underscore.
    Additionally, LLVM provides some "explicit" symbols which would
    otherwise only exist in form of a macro (see comment in
    llvm-src/lib/Support/Windows/DynamicLibrary.inc). These are also only
    available in unmangled form, for example "sinf".
    Thus I propose to add this code at the end of
    "KaleidoscopeJIT::findMangledSymbol":

> #ifdef LLVM_ON_WIN32
>     // For Windows retry without "_" at begining, as RTDyldMemoryManager uses
>     // GetProcAddress and standard libraries like msvcrt.dll use names
>     // with and without "_" (for example "_itoa" but "sin").
>     if (Name.length() > 2 && Name[0] == '_')
>       if (auto SymAddr = RTDyldMemoryManager::getSymbolAddressInProcess(
>             Name.substr(1)))
>         return JITSymbol(SymAddr, JITSymbolFlags::Exported);
> #endif
>
>     return nullptr;
>   }

  - "ready> sin(1.0);
     Read top-level expression:
     define double @2() {
     entry:
       ret double 0x3FEAED548F090CEE
     }

     Evaluated to 0.841471":
    Actually "sin" is not called at all, LLVM knew "sin" and just returns
    the already const-folded value.
    In my case, I do see a
    "%calltmp = call double @sin(double 1.000000e+00)",
    though, inspite of the same optimizations as in your listing...

  - "ready> def foo(x) sin(x)*sin(x) + cos(x)*cos(x);
     Read function definition:
     define double @foo(double %x) {
     entry:
       %calltmp = call double @sin(double %x)
       %multmp = fmul double %calltmp, %calltmp
       %calltmp2 = call double @cos(double %x)
       %multmp4 = fmul double %calltmp2, %calltmp2
       %addtmp = fadd double %multmp, %multmp4
       ret double %addtmp
     }":
    In my case, the CSE does not work, although it does work for the
    example in 4.3:
     "ready> def foo(x) sin(x)*sin(x) + cos(x)*cos(x);
     ready> Read function definition:
     define double @foo(double %x) {
     entry:
       %calltmp = call double @sin(double %x)
       %calltmp1 = call double @sin(double %x)
       %multmp = fmul double %calltmp, %calltmp1
       %calltmp2 = call double @cos(double %x)
       %calltmp3 = call double @cos(double %x)
       %multmp4 = fmul double %calltmp2, %calltmp3
       %addtmp = fadd double %multmp, %multmp4
       ret double %addtmp
     }"

  - "extern putchard(x); putchard(120);":
    Again, on Windows, this doesn't work. To make it work, you have to
    use "extern "C" __declspec(dllexport) double putchard(double X)"
    so the function will be found via GetProcAddress for the main module.
    This wouldn't hurt for Linux / Mac, would it?

Best regards
Moritz Kroll