[PATCH] Add a jumptable attribute and support for creating jump-instruction tables

Nick Lewycky nlewycky at google.com
Tue Jun 3 15:37:57 PDT 2014


On 30 May 2014 11:35, Tom Roeder <tmroeder at google.com> wrote:

> Ping
>

+  typedef struct TableMeta {
+    /// The number of this table
+    unsigned TableNum;
+
+    /// The current number of jump entries in the table.
+    unsigned Count;
+  } TableMetadata;

Optional: In C++ we prefer "struct TableMetadata { ... };". You don't seem
to use the TableMeta name anyways.

+ if (JITI && JITI->getTables().size() > 0) {

Optional: DenseMap has an empty() method.

bool JumpInstrTables::hasTable(FunctionType *FunTy) {
  FunctionType *TransTy = transformType(FunTy);
  JumpMap::iterator it = Metadata.find(TransTy);
  return Metadata.end() != it;
}

Optional: "return Metadata.find(TransTy) != Metadata.end();" ? At lease use
"auto" instead of "JumpMap::iterator".

+ static cl::opt<bool>
+ BuildJumpInstrTables(
+     "build-jump-instr-tables", cl::Hidden, cl::init(false),
+     cl::desc("Build jump-instruction tables for address-taken
functions"));

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.


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.

LGTM conditional on the removal of BuildJumpInstrTables global.

On Fri, May 23, 2014 at 4:07 PM, Tom Roeder <tmroeder at google.com> wrote:
> > 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.
> >
> > http://reviews.llvm.org/D3361
> >
> > Files:
> >   docs/LangRef.rst
> >   include/llvm-c/Core.h
> >   include/llvm/Analysis/JumpInstrTableInfo.h
> >   include/llvm/Analysis/Passes.h
> >   include/llvm/Bitcode/LLVMBitCodes.h
> >   include/llvm/CodeGen/JumpInstrTables.h
> >   include/llvm/CodeGen/Passes.h
> >   include/llvm/IR/Attributes.h
> >   include/llvm/InitializePasses.h
> >   include/llvm/LinkAllPasses.h
> >   include/llvm/Target/TargetInstrInfo.h
> >   lib/Analysis/Analysis.cpp
> >   lib/Analysis/CMakeLists.txt
> >   lib/Analysis/JumpInstrTableInfo.cpp
> >   lib/AsmParser/LLLexer.cpp
> >   lib/AsmParser/LLParser.cpp
> >   lib/AsmParser/LLToken.h
> >   lib/Bitcode/Reader/BitcodeReader.cpp
> >   lib/Bitcode/Writer/BitcodeWriter.cpp
> >   lib/CodeGen/AsmPrinter/AsmPrinter.cpp
> >   lib/CodeGen/CMakeLists.txt
> >   lib/CodeGen/JumpInstrTables.cpp
> >   lib/CodeGen/LLVMTargetMachine.cpp
> >   lib/IR/Attributes.cpp
> >   lib/IR/Verifier.cpp
> >   lib/LTO/LTOCodeGenerator.cpp
> >   lib/Target/ARM/ARMBaseInstrInfo.cpp
> >   lib/Target/ARM/ARMBaseInstrInfo.h
> >   lib/Target/X86/X86InstrInfo.cpp
> >   lib/Target/X86/X86InstrInfo.h
> >   lib/Transforms/IPO/IPO.cpp
> >   test/Bitcode/attributes.ll
> >   test/CodeGen/ARM/jump_tables.ll
> >   test/CodeGen/X86/jump_table_alias.ll
> >   test/CodeGen/X86/jump_table_bitcast.ll
> >   test/CodeGen/X86/jump_tables.ll
> >   test/Verifier/jumptable.ll
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140603/379c4197/attachment.html>


More information about the llvm-commits mailing list