[llvm-dev] Kaleidoscope tutorial: comments, corrections and Windows support
Moritz Kroll via llvm-dev
llvm-dev at lists.llvm.org
Sat Feb 11 10:15:02 PST 2017
Hi Mehdi,
I submitted a patch:
[PATCH] D29864: Update Kaleidoscope tutorial and improve Windows support
https://reviews.llvm.org/D29864
An open question: Why doesn't CSE work for sin and cos as shown at the
end of section 4.4?
It looks like, they are not recognized as intrinsics and don't get the
according attributes in Function::Function.
Function::recalculateIntrinsicID requires the function name to start
with "llvm.".
Maybe this worked in a previous version of LLVM?
Optimizing sin and cos is specifically mentioned in AliasAnalysis.h:
llvm-3.9.1.src\include\llvm\Analysis\AliasAnalysis.h:
/// Checks if the specified call is known to never read or write memory.
///
/// Note that if the call only reads from known-constant memory, it
is also
/// legal to return true. Also, calls that unwind the stack are legal for
/// this predicate.
///
/// Many optimizations (such as CSE and LICM) can be performed on
such calls
/// without worrying about aliasing properties, and many calls have this
/// property (e.g. calls to 'sin' and 'cos').
///
/// This property corresponds to the GCC 'const' attribute.
bool doesNotAccessMemory(ImmutableCallSite CS) {
return getModRefBehavior(CS) == FMRB_DoesNotAccessMemory;
}
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
>
>
More information about the llvm-dev
mailing list