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

Nick Lewycky nlewycky at google.com
Wed May 7 20:15:16 PDT 2014

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.

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?

+    // Don't do the replacement if this use is a direct call to this
+​    if ((CS.isCall() || CS.isInvoke()) && !isIndirectCall(CS)) {
+​      // If the use is not the called value, then replace it.
+​      if (CS.getCalledValue() != GV) {
+​        U->set(V);
+​        return true;
+      }

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

"CS.isCall() || CS.isInvoke()" simplifies to "CS" used in a bool context.

+  // Go through all uses of this function and replace the uses of GV with
+  // jump-table version of the function. Get the uses as a vector before
+  // replacing them, since replacing them changes the use list and
+  // the iterator otherwise.
+  std::vector<Use*> Uses;
+  for (Use &U : GV->uses())
+    Uses.push_back(&U);
+  for(Use *U : Uses)
+    replaceGlobalValueIndirectUse(GV, F, U);

Missing space after for.

The common way to handle the invalidation is more like:

  for (Value::use_iterator I = GV->use_begin(), E = GV->use_end(); I != E;)
    Use &U = *UI++;
    replaceGlobalValueIndirectUse(GV, F, U);

though that can probably be C++11ified now.

+ Type *Int32Ty = Type::getInt32Ty(getGlobalContext());

Nope. Let me explain what a context is for.

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.

The context for a module must match the context for everything inside it.

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.

+ return Functions.size() > 0;

DenseMap has an empty() method.



More information about the llvm-commits mailing list