[llvm-commits] [llvm] r91626 - in /llvm/trunk: lib/ExecutionEngine/JIT/JIT.cpp lib/ExecutionEngine/JIT/JIT.h lib/ExecutionEngine/JIT/JITEmitter.cpp unittests/ExecutionEngine/JIT/JITTest.cpp

[Jeffrey Yasskin]

On Sun, Dec 20, 2009 at 4:07 AM, nicolas geoffray
<nicolas.geoffray at gmail.com> wrote:
> Hi Jeffrey,
>
>>
>>
>> ==============================================================================
>> --- llvm/trunk/lib/ExecutionEngine/JIT/JITEmitter.cpp (original)
>> +++ llvm/trunk/lib/ExecutionEngine/JIT/JITEmitter.cpp Thu Dec 17 15:35:29
>> 2009
>> @@ -517,9 +517,15 @@
>>   void *Actual = TheJIT->isCompilingLazily()
>>     ? (void *)(intptr_t)LazyResolverFn : (void *)0;
>>
>> +  // TODO: Delete this when PR5737 is fixed.
>> +  std::string ErrorMsg;
>> +  if (TheJIT->materializeFunction(F, &ErrorMsg)) {
>> +    llvm_report_error("Error reading function '" + F->getName()+
>> +                      "' from bitcode file: " + ErrorMsg);
>> +  }
>
> This should be guarded by a lazy compilation enabled check.

What's the actual failure you see? As far as I understand, it's lazy
bitcode loading that causes functions to have ghost linkage, not lazy
compilation.

Until http://llvm.org/bugs/show_bug.cgi?id=5737 is fixed, there's no
way to tell whether a ghost function is available_externally without
materializing it, so I'd expect not having this to break all users of
lazy bitcode loading, not just users who also compile lazily.

>>   // If this is an external declaration, attempt to resolve the address
>> now
>>   // to place in the stub.
>> -  if (F->isDeclaration() && !F->hasNotBeenReadFromBitcode()) {
>> +  if (F->isDeclaration() || F->hasAvailableExternallyLinkage()) {
>
> Why remove the hasNotBeenReadFromBitcode? The hasNotBeenReadFromBitcode
> function verifies that the linkage of the function is GhostLinkage, which is
> used by lazy compilation. So users that use lazy compilation require the
> call to hasNotBeenReadFromBitcode.

Given the above call to materializeFunction(F),
hasNotBeenReadFromBitcode should always be false, so testing it is
redundant. I could add an assert() if you want.

>>     Actual = TheJIT->getPointerToFunction(F);
>>
>>     // If we resolved the symbol to a null address (eg. a weak external)
>> @@ -552,7 +558,7 @@
>>   // exist yet, add it to the JIT's work list so that we can fill in the
>> stub
>>   // address later.
>>   if (!Actual && !TheJIT->isCompilingLazily())
>> -    if (!F->isDeclaration() || F->hasNotBeenReadFromBitcode())
>> +    if (!F->isDeclaration() && !F->hasAvailableExternallyLinkage())
>>       TheJIT->addPendingFunction(F);
>
> Likewise.
>
>>
>>   return Stub;
>> @@ -755,9 +761,16 @@
>>     void *ResultPtr = TheJIT->getPointerToGlobalIfAvailable(F);
>>     if (ResultPtr) return ResultPtr;
>>
>> +    // TODO: Delete this when PR5737 is fixed.
>> +    std::string ErrorMsg;
>> +    if (TheJIT->materializeFunction(F, &ErrorMsg)) {
>> +      llvm_report_error("Error reading function '" + F->getName()+
>> +                        "' from bitcode file: " + ErrorMsg);
>> +    }
>> +
>
> Add the guard for enabled lazy compilation.
>
>>
>>     // If this is an external function pointer, we can force the JIT to
>>     // 'compile' it, which really just adds it to the map.
>> -    if (F->isDeclaration() && !F->hasNotBeenReadFromBitcode())
>> +    if (F->isDeclaration() || F->hasAvailableExternallyLinkage())
>>       return TheJIT->getPointerToFunction(F);
>>   }
>
> Don't remove the hasNotBeenReadFromBitcode.
>
> Hope that still fixes the PR!
> Cheers,
> Nicolas
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>