<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 30 May 2014 11:35, 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">
Ping<br></blockquote><div><br></div><div>+ typedef struct TableMeta {</div><div>+ /// The number of this table</div><div>+ unsigned TableNum;</div><div>+</div><div>+ /// The current number of jump entries in the table.</div>
<div>+ unsigned Count;</div><div>+ } TableMetadata;</div><div><br></div><div>Optional: In C++ we prefer "struct TableMetadata { ... };". You don't seem to use the TableMeta name anyways.</div><div><br></div>
<div>+ if (JITI && JITI->getTables().size() > 0) {</div><div><br></div><div>Optional: DenseMap has an empty() method.</div><div><br></div><div><div>bool JumpInstrTables::hasTable(FunctionType *FunTy) {</div>
<div> FunctionType *TransTy = transformType(FunTy);</div><div> JumpMap::iterator it = Metadata.find(TransTy);</div><div> return Metadata.end() != it;</div><div>}</div></div><div><br></div><div>Optional: "return Metadata.find(TransTy) != Metadata.end();" ? At lease use "auto" instead of "JumpMap::iterator".</div>
<div><br></div><div><div>+ static cl::opt<bool></div><div>+ BuildJumpInstrTables(</div><div>+ "build-jump-instr-tables", cl::Hidden, cl::init(false),</div><div>+ cl::desc("Build jump-instruction tables for address-taken functions"));</div>
</div><div><br></div><div>No. Firstly, such globals are forbidden on the grounds that they're thread-unsafe, consider TargetOptions instead. Yes we still have some, no you can't add more. It looks like this controls whether we actually do the jump-table substitution. Please remove it, replacing it with true. We already have a control for whether to do jump instruction tables, and that's the presence or absence of the "jumptable" attribute on each function.</div>
<div><br></div><div><br></div><div>On the one hand I'm a bit concerned about the code in AsmPrinter.cpp not being portable enough (the rest of that file does not care whether we're emitting thumb or not for example), yet I don't see a better way to do it. And we do need to know since we're emitting the thunks here. One way to do this might be to sink it through the MCStreamer interface, but that just seems worse. I'm going to okay the patch in this state and let reviewers comment on it in post-commit review. Please be responsive when they do.<br>
</div><div><br></div><div>LGTM conditional on the removal of BuildJumpInstrTables global.</div><div><br></div><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">
<div class=""><div class="h5">On Fri, May 23, 2014 at 4:07 PM, Tom Roeder <<a href="mailto:tmroeder@google.com">tmroeder@google.com</a>> wrote:<br>
> This version of the patch fixes the problems from the most recent review. It also adds the requirement that jumptable functions must also be unnamed_addr; this is now enforced by the Verifier. PTAL.<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-c/Core.h<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_alias.ll<br>
> test/CodeGen/X86/jump_table_bitcast.ll<br>
> test/CodeGen/X86/jump_tables.ll<br>
> test/Verifier/jumptable.ll<br>
</div></div></blockquote></div><br></div></div>