Hi Garrison,<br><br>I am not a specialist of the code but here is my 2 cents:<br><br>- I like the idea that in lazy-mode the call (in module or not) is treated by a stub (like every calls).<br><br>- If the jit is in non-lazy mode, I'm not really fan of the "stub" solution. Is it not possible to use the same mechanism as it already exists : add the function to pending list and emit it after the current functions ? In fact, can you replace forEmitFunctionStub with getLazyFunctionStub ? With a non-lazy jit, I want the first execution of my function to be the same execution speed as the second one. A stub in non-lazy mode doesn't fell natural to me.<br>
<br>- I don't see why functions external to every modules (let's say "printf" ?) are delayed with your mechanism. In the last part of your modified getPointerToFunction, you call "getPointerToNamedFunction". AFAICT a printf call will be catch by this code, no ?<br>
<br>- I like the ideas to be flexible, to be able to say this global variable declaration should point here (with addGlobalMapping), even if there is another module which have a global variable. And in fact, as the workaround is relatively easy, I wonder if it worth the change ?<br>
<br>Anyway, thanks for asking opinion on this and for contributions you've made on code !<br><br>Olivier.<br><br><br><div class="gmail_quote">On Tue, Feb 16, 2010 at 2:10 PM, Garrison Venn <span dir="ltr"><<a href="mailto:gvenn.cfe.dev@gmail.com">gvenn.cfe.dev@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div style="word-wrap: break-word;">The patch I recently attached to bug <span style="font-family: sans-serif;"><a href="http://llvm.org/bugs/show_bug.cgi?id=2606" style="color: rgb(102, 51, 102);" target="_blank">2606</a>, reproduced here, is my first attempt to solve this issue</span><div>
<font face="sans-serif"><span>as represented by the attached test cases. To solve the foreign Module GlobalVariable problem,</span></font></div><div><font face="sans-serif"><span>I modified JIT::getOrEmitGlobalVariable(...) to directly attempt to map a found "external" GlobalVariable.</span></font></div>
<div><font face="sans-serif"><span>To solve the foreign Module Function problem, I modified both JIT.{h,cpp} and JITEmitter.cpp to emit</span></font></div><div><font face="sans-serif"><span>a stub for a found "external" Function instance, when that foreign Module defined Function has not</span></font></div>
<div><font face="sans-serif"><span>yet been compiled.</span></font></div><div><font face="sans-serif"><span><br></span></font></div><div><font face="sans-serif"><span>All areas of interest are marked with // GMV Mod:</span></font></div>
<div><font face="sans-serif"><span><br></span></font></div><div><font face="sans-serif"><span>As I've taken liberties with existing code, and thereby may have touched test cases that will fail, I would like</span></font></div>
<div><font face="sans-serif"><span>to have those members of the list that have time, review this proposal for comments/changes.</span></font></div><div><font face="sans-serif"><span><br></span></font></div><div><font face="sans-serif"><span>Major mods:</span></font></div>
<div><font face="sans-serif"><span><br></span></font></div><div><font face="sans-serif"><span>JIT.{h,cpp}:</span></font></div><div><font face="sans-serif"><span><br></span></font></div><div><font face="sans-serif"><span>1)<span style="white-space: pre;"> </span>I included Module.h.</span></font></div>
<div><font face="sans-serif"><span><br></span></font></div><div><font face="sans-serif"><span>2)<span style="white-space: pre;"> </span>JIT::<span style="white-space: pre;">getPointerToFunction</span>(...) was modified to search for a non-mapped function in all "other" modules.</span></font></div>
<div><font face="sans-serif"><span><span style="white-space: pre;"> </span>If found a stub is emitted, which will lazily emit the real function at runtime.</span></font></div><div><font face="sans-serif"><span><br></span></font></div>
<div><font face="sans-serif"><span><span style="white-space: pre;"> </span>One issue here is that functions that are external to all modules, have their mappings delayed by the</span></font></div><div><font face="sans-serif"><span><span style="white-space: pre;"> </span>above loop. I also kept the hasAvailableExternallyLinkage() the same as before, as I'm not sure if this</span></font></div>
<div><font face="sans-serif"><span><span style="white-space: pre;"> </span>should also go through the same module procedure. The stub emission will only take place for function</span></font></div><div><font face="sans-serif"><span><span style="white-space: pre;"> </span>definitions that are marked with external linkage. Should other cases be allowed? Should visibility also </span></font></div>
<div><font face="sans-serif"><span><span style="white-space: pre;"> </span>be considered?</span></font></div><div><font face="sans-serif"><span><br></span></font></div><div><font face="sans-serif"><span>3)<span style="white-space: pre;"> JIT::getOrEmitGlobalVariable</span>(...) was modified to search for non-mapped global variables in all "other"</span></font></div>
<div><font face="sans-serif"><span><span style="white-space: pre;"> </span>modules. If found, the address of the foreign global variable is requested, and mapped. There is no</span></font></div><div><font face="sans-serif"><span><span style="white-space: pre;"> </span>delay here.</span></font></div>
<div><font face="sans-serif"><span><br></span></font></div><div><font face="sans-serif"><span><span style="white-space: pre;"> </span>An attempt is first made to find the global outside of all modules, before attempting to find the function</span></font></div>
<div><font face="sans-serif"><span><span style="white-space: pre;"> </span>in the known modules. This is the reverse logic used in #2. On searching, unlike for functions, a linkage </span></font></div><div><font face="sans-serif"><span><span style="white-space: pre;"> </span>check was not made. Also the global variable declaration and definition must have the exact same type. </span></font></div>
<div><font face="sans-serif"><span><span style="white-space: pre;"> </span>Is this the correct approach?</span></font></div><div><font face="sans-serif"><span><br></span></font></div><div><font face="sans-serif"><span>4) The declaration of void* forceEmitFunctionStub(Function *F) was added to the class JIT. This is </span></font></div>
<div><font face="sans-serif"><span><span style="white-space: pre;"> </span>implemented in JITEmitter.cpp.</span></font></div><div><font face="sans-serif"><span><br></span></font></div><div><span style="font-family: sans-serif;">JITEmitter.cpp:</span></div>
<div><font face="sans-serif"><span><br></span></font></div><div><font face="sans-serif"><span>1)<span style="white-space: pre;"> JIT::forceEmitFunctionStub</span>(...) was added. It turns around and calls JITResolver::<span style="font-family: Helvetica;"><span style="font-family: sans-serif;"><span style="white-space: pre;">forceEmitFunctionStub</span>(...).</span></span></span></font></div>
<div><font face="sans-serif"><span><br></span></font></div><div><font face="sans-serif"><span><span style="white-space: pre;"> </span>Is the JIT:getPointerToFunction(...) code path sensitive to such call overhead?</span></font></div>
<div><font face="sans-serif"><span><br></span></font></div><div><font face="sans-serif"><span>2)<span style="white-space: pre;"> JITResolver::forceEmitFunctionStub(Function *F)</span> was added. It is a simplified version of JITResolver::getLazyFunctionStub(...),</span></font></div>
<div><font face="sans-serif"><span><span style="white-space: pre;"> </span>which does not add to pending. It uses the same JITResolver::JITCompilerFn(...) for runtime emission that </span></font></div><div><font face="sans-serif"><span><span style="white-space: pre;"> </span>the lazy compilation system uses.</span></font></div>
<div><font face="sans-serif"><span><br></span></font></div><div><font face="sans-serif"><span><span style="white-space: pre;"> </span>Outside of the goal, I'm not sure what code is unnecessary here. Notice that the hasAvailableExternallyLinkage</span></font></div>
<div><font face="sans-serif"><span><span style="white-space: pre;"> </span>case is dealt with here even though <span style="white-space: pre;">forceEmitFunctionStub</span>(...) would not be entered for this case. I'm forced to </span></font></div>
<div><font face="sans-serif"><span><span style="white-space: pre;"> </span>use JITResolver::JITCompilerFn(...) as there is a static relationship between JITResolver and say X86JITInfo. See</span></font></div><div><font face="sans-serif"><span><span style="white-space: pre;"> </span>implementation of X86JITInfo::getLazyResolverFunction(...) (X86JITInfo.cpp). Should a new JITCompilerFn like </span></font></div>
<div><font face="sans-serif"><span><span style="white-space: pre;"> </span>function be created for the purposes of resolving this issue? This would of course require modifications to the </span></font></div><div><font face="sans-serif"><span><span style="white-space: pre;"> </span>subclasses of TargetJITInfo.</span></font></div>
<div><font face="sans-serif"><span><br></span></font></div><div><font face="sans-serif"><span>3)<span style="white-space: pre;"> JITResolver::JITCompilerFn</span>(...) was modified to no longer exit on being used for non-lazy compilation scenarios.</span></font></div>
<div><font face="sans-serif"><span><br></span></font></div><div><font face="sans-serif"><span><span style="white-space: pre;"> </span>This means of course that the error test path behavior for this function has been modified. See #2 above.</span></font></div>
<div><font face="sans-serif"><span><br></span></font></div><div><font face="sans-serif"><span>Beyond any issues with the patch, there is a question, in my mind, as to whether <a href="http://llvm.org/bugs/show_bug.cgi?id=2606" style="color: rgb(102, 51, 102);" target="_blank">2606</a> is really a bug. Sure its resolution makes </span></font></div>
<div><font face="sans-serif"><span>using the JIT simpler for cross module behavior, but current manual solutions may be more fined grained in their approach. </span></font></div><div><font face="sans-serif"><span>If so a fix to <a href="http://llvm.org/bugs/show_bug.cgi?id=2606" style="color: rgb(102, 51, 102);" target="_blank">2606</a> would circumvent such handling.</span></font></div>
<div><font face="sans-serif"><span><br></span></font></div><div><font face="sans-serif"><span>Thanks for any time spent on this</span></font></div><div><font face="sans-serif"><span><br></span></font></div><font color="#888888"><div>
<font face="sans-serif"><span>Garrison</span></font></div><div><font face="sans-serif"><span><br></span></font></div><div><font face="sans-serif"><span></span></font></div></font></div><br><div style="word-wrap: break-word;">
<div><font face="sans-serif"><span></span></font></div><div><font face="sans-serif"><span><br></span></font></div><div><font face="sans-serif"><span><span style="white-space: pre;"> </span></span></font></div><div><font face="sans-serif"><span><br>
</span></font></div><div><font face="sans-serif"><span><br></span></font></div></div><br>_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:LLVMdev@cs.uiuc.edu">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
<br></blockquote></div><br>