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

Tue Feb 7 14:56:01 PST 2017

Hi Mehdi,

well, some comments are actually questions, but I'll see what I can do.
For Chapter 5, I'm already changing the tutorial directly based on svn 
trunk.

Best regards
Moritz

On 07.02.2017 08:02, Mehdi Amini wrote:
>
>> On Feb 6, 2017, at 5:07 PM, Moritz Kroll via llvm-dev <llvm-dev at lists.llvm.org> wrote:
>>
>> 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:
>
> Any chance you would submit a patch to fix all this?
>
> http://llvm.org/docs/DeveloperPolicy.html
> http://llvm.org/docs/Phabricator.html
>
> Thanks,
>
> —
> Mehdi
>
>
>>
>> - "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
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>