[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 Mon, Dec 21, 2009 at 2:00 PM, nicolas geoffray
<nicolas.geoffray at gmail.com> wrote:
> Hi Jeffrey,
>
>>
>> > 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.
>
> Yes, sorry, that was lazy bitcode loading. This should not be triggered
> here.
>>
>> 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.
>>
>
> Ah, that's unfortunate. Two things incompatible with each other....  Still,
> the behavior was OK before your change, so I guess both ways do not fix
> PR5737. I'd really like to see the old behavior coming back again until a
> proper fix to PR5737 is submitted.

The behavior was not ok before my change: available_externally
functions loaded lazily from bitcode would infinite-loop if you ever
actually called them. This change avoided that infinite loop at the
cost of loading one extra layer of functions from bitcode. It doesn't
load the whole transitive call graph, just one layer down from the
function you actually need to compile, and unless I did something
wrong it doesn't compile that extra layer. Nick and I intend to fix
PR5737 before 2.7 comes out, but trading some extra memory and time to
get rid of an infinite loop seemed like a decent short-term option.

Now, if it's costing vmkit more memory or time than I expected, I'm
certainly willing to reconsider the order of these two fixes. What did
this actually break for you? How much extra time or memory is it
costing?

>> Declaration() || 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.
>>
>
> No, the matierialize function should not be there in the first place.

When I delete it, I'll put back the hasNotBeenRead check. (I expect
the tests will force me to.)