<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 14 May 2014 11:57, 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 diff fixes the problems noted in the review. PTAL.<br>
<br>
For the for-loop invalidation problem, I didn't see any way to increment the implicit iterator early in C++11 style, so I've gone back to a standard iterator for loop to match the style of other invalidation-sensitive Use-handling loops.<br>


<br>
Also, as I was working on these fixes, I found a problem with GlobalAlias: it doesn't support replaceUsesOfWithOnConstant, and it cannot target declaration-only functions. So, I've added some special-case handling for GlobalAlias statements that target jumptable functions. I've also added a new test case for "alias".<br>

</blockquote><div><br></div><div>Rafael, could you take a look at how this interacts with global aliases?</div><div><br></div><div>+//===- JumpInstrTableInfo.h: Info for Jump-Instruction Tables -*- C++ -*---===//<br></div>

<div>...</div><div>+//===----- JumpInstrTables.h: Jump-Instruction Tables -*- C++ -*-----------===//<br></div><div><br></div><div>Please right-align the emacs major mode marker.</div><div><br></div><div>+  virtual ~JumpInstrTables() { }<br>

</div><div><br></div><div>Move the implementation into a .cpp file. In this case, the destructor is the key method which means that it controls which TUs will contain the functions and debug info. Putting it in the header file means that all inclusions of it will contain copies, but if you move it to the .cpp then we get one copy in that .cpp.</div>

<div><br></div><div><div>+JumpInstrTableInfo::JumpInstrTableInfo()</div><div>+  : ImmutablePass(ID),</div><div>+  Tables() {</div></div><div><br></div><div>Formatting? Tables() should be on the previous line? In fact, doesn't </div>

<div>  JumpInstrTableInfo::JumpInstrTableInfo() : ImmutablePass(ID), Tables() {<br></div><div>fit on one line?</div><div><br></div><div>+//===- JumpInstrTables.cpp: Jump-Instruction Tables -*- C++ -*-------------===//<br>

</div><div><br></div><div>Right-align the -*- again.</div><div><br></div><div><div>+  Value::use_iterator I, E;</div><div>+  for (I = GV->use_begin(), E = GV->use_end(); I != E;) {</div></div><div><br></div><div>Any reason that isn't:</div>

<div>  for (Value::use_iterator I = GV->use_begin(), E = GV->use_end(); I != E;) {<br></div><div>?</div><div><br></div><div><div>+JumpInstrTables::JumpInstrTables()</div><div>+  : ModulePass(ID),</div><div>+  Metadata(),</div>

<div>+  JITI(nullptr),</div><div>+  TableCount(0) {</div></div><div><br></div><div>Again, should be packed formatting here. You might want to try clang-format --style=LLVM?</div><div><br></div><div><br></div><div>This is missing a change to include/llvm-c/Core.h in the commented out section of LLVMAttribute.</div>

<div><br></div><div>I'd also appreciate someone from the backend side looking at this.</div><div><br></div><div>Nick</div><div> </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><div class="h5"><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>
</div></div>  test/CodeGen/X86/jump_table_alias.ll<br>
  test/CodeGen/X86/jump_table_bitcast.ll<br>
  test/CodeGen/X86/jump_tables.ll<br>
</blockquote></div><br></div></div>