<div dir="ltr"><div><div><div>So first, a design comment. The 'jumptable' attribute itself has no IR meaning. It doesn't change anything about the execution of the IR, it doesn't change whether IR is valid, even. It appears to be an ABI attribute.</div>
<div><br></div><div>What's the right behaviour if you link together two modules where a function is not "jumptable" in one but is in the other? Is it safe to apply "jumptable" to a module that didn't have it? It looks like not. I think the answer is that two modules needed to both have this attribute set the same way in the two modules before merging, and that this is reasonable because it's an ABI issue. Similarly, if you generated those two modules into .o files, they wouldn't be ABI compatible in machine code either, right?</div>
<div><br></div><div>+ // Don't do the replacement if this use is a direct call to this function.</div><div><span style="white-space:pre-wrap">+</span> if ((CS.isCall() || CS.isInvoke()) && !isIndirectCall(CS)) {</div>
<div><span style="white-space:pre-wrap">+</span> // If the use is not the called value, then replace it.</div><div><span style="white-space:pre-wrap">+</span> if (CS.getCalledValue() != GV) {</div><div>
<span style="white-space:pre-wrap">+</span> U->set(V);</div><div><span style="white-space:pre-wrap">+</span> return true;</div><div><span style="white-space:pre-wrap">+</span> }</div><div><br></div>
</div><div>I think you have a bug here when the value is used both as the callee and is also an argument. Instead you can use CS.isCallee(U); to determine whether this particular use of your value is a use as the callee operand or not.</div>
<div><br></div><div>"CS.isCall() || CS.isInvoke()" simplifies to "CS" used in a bool context.</div>
<div><br></div><div><div>+ // Go through all uses of this function and replace the uses of GV with the</div><div>+ // jump-table version of the function. Get the uses as a vector before</div><div>+ // replacing them, since replacing them changes the use list and invalidates</div>
<div>+ // the iterator otherwise.</div><div>+ std::vector<Use*> Uses;</div><div>+ for (Use &U : GV->uses())</div><div>+ Uses.push_back(&U);</div><div>+</div><div>+ for(Use *U : Uses)</div><div>+ replaceGlobalValueIndirectUse(GV, F, U);</div>
</div></div><div><br></div><div>Missing space after for.</div><div><br></div><div>The common way to handle the invalidation is more like:</div><div><br></div><div> for (Value::use_iterator I = GV->use_begin(), E = GV->use_end(); I != E;) {</div>
<div> Use &U = *UI++;</div><div> replaceGlobalValueIndirectUse(GV, F, U);</div><div> }</div><div><br></div><div>though that can probably be C++11ified now.</div><div><br></div>+ Type *Int32Ty = Type::getInt32Ty(getGlobalContext());<div>
<br></div><div>Nope. Let me explain what a context is for.</div><div><br></div><div>Suppose you have a game, and it uses a graphics library and it uses a 3d audio library. Both of those in turn use llvm under the hood, but they aren't aware of each other's existence. The purpose of the llvm context is to keep "global" (or at least, "outside the Module") issues out of the program-level global context, so that these two users can act independently and not interfere with each other in any way.</div>
<div><br></div><div>The context for a module must match the context for everything inside it.</div><div><br></div><div>Finally, they're used for supporting threading. Types are pointer comparable, if you construct the same type twice you get the same Type*. The mapping that holds these is not locked, it's assumed that you only do one operation on them at a time per-thread. If someone were to create two threads each with their own LLVMContext to keep things separate, they couldn't both call JumpInstrTables::transformType() since your function would cause racy access to the internal map inside the global context.</div>
<div><br></div>+ return Functions.size() > 0;<div><br></div><div>DenseMap has an empty() method.</div><div><br></div><div>Nick</div><div>
<div><div class="gmail_extra"><br><br><div class="gmail_quote">On 11 April 2014 15:00, Tom Roeder <span dir="ltr"><<a href="mailto:tmroeder@google.com" target="_blank">tmroeder@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">This patch provides a new jumptable attribute and associated CodeGen support to generate jump-instruction tables and replace function references for jumptable-annotated functions. The work of generating the tables is now done with an IR pass in CodeGen to replace address-taken functions, along with a change to AsmPrinter to generate the assembly for the tables. This is a followup to the discussion on llvmdev. See <a href="http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-April/071866.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-April/071866.html</a><br>
<br>
This version of the patch has support for X86 and ARM backends, along with tests for each.<br>
<br>
<a href="http://reviews.llvm.org/D3361" target="_blank">http://reviews.llvm.org/D3361</a><br>
<br>
Files:<br>
docs/LangRef.rst<br>
include/llvm/Analysis/JumpInstrTableInfo.h<br>
include/llvm/Analysis/Passes.h<br>
include/llvm/Bitcode/LLVMBitCodes.h<br>
include/llvm/CodeGen/JumpInstrTables.h<br>
include/llvm/CodeGen/Passes.h<br>
include/llvm/IR/Attributes.h<br>
include/llvm/InitializePasses.h<br>
include/llvm/LinkAllPasses.h<br>
include/llvm/Target/TargetInstrInfo.h<br>
lib/Analysis/Analysis.cpp<br>
lib/Analysis/CMakeLists.txt<br>
lib/Analysis/JumpInstrTableInfo.cpp<br>
lib/AsmParser/LLLexer.cpp<br>
lib/AsmParser/LLParser.cpp<br>
lib/AsmParser/LLToken.h<br>
lib/Bitcode/Reader/BitcodeReader.cpp<br>
lib/Bitcode/Writer/BitcodeWriter.cpp<br>
lib/CodeGen/AsmPrinter/AsmPrinter.cpp<br>
lib/CodeGen/CMakeLists.txt<br>
lib/CodeGen/JumpInstrTables.cpp<br>
lib/CodeGen/LLVMTargetMachine.cpp<br>
lib/IR/Attributes.cpp<br>
lib/IR/Verifier.cpp<br>
lib/LTO/LTOCodeGenerator.cpp<br>
lib/Target/ARM/ARMBaseInstrInfo.cpp<br>
lib/Target/ARM/ARMBaseInstrInfo.h<br>
lib/Target/X86/X86InstrInfo.cpp<br>
lib/Target/X86/X86InstrInfo.h<br>
lib/Transforms/IPO/IPO.cpp<br>
test/Bitcode/attributes.ll<br>
test/CodeGen/ARM/jump_tables.ll<br>
test/CodeGen/X86/jump_table_bitcast.ll<br>
test/CodeGen/X86/jump_tables.ll<br>
<br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div></div></div></div>