[PATCH] D42453: Use branch funnels for virtual calls when retpoline mitigation is enabled.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc requested changes to this revision.
chandlerc added a comment.
This revision now requires changes to proceed.

Did a pass over the code. Really only minor nits and naming questions. Generally, the structure and such makes a lot of sense to me now. Thanks so much for working on threading this through all the layers so nicely!

Happy to take another look when you get tests and such updated.



================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:607-608
     SingleImpl, ///< Single implementation devirtualization
+    JumpTable,  ///< When retpoline mitigation is enabled, use a jump table that
+                ///< is defined in the merged module. Otherwise same as Indir.
   } TheKind = Indir;
----------------
Is it really a jump table though? You use this term throughout, so only commenting here, but I wonder if there is a better term that makes it more clear what is going on here. I'm worried people will assume this is actually implemented with a jump table as opposed to a branch funnel (or binary-ish search).


================
Comment at: llvm/include/llvm/Target/Target.td:1146
+  let InOperandList = (ins variable_ops);
+  let AsmString = "";
+  let hasSideEffects = 1;
----------------
Does it make sense to put a comment string here to make reading the dump of MI easier? (I'm not sure, genuine question here.)


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6049
+  case Intrinsic::icall_jumptable: {
+    std::vector<SDValue> Ops;
+    Ops.push_back(DAG.getRoot());
----------------
SmallVector? Seems likely to have lots of cases where 8 or 16 would handle..


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6064
+    };
+    std::vector<JumpTableTarget> Targets;
+
----------------
Same question here.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6070-6071
+      if (ElemBase != Base)
+        report_fatal_error("all llvm.icall.jumptable operands must refer to "
+                           "the same GlobalValue");
+
----------------
Is it worth teaching the verifier about this and the other invariants below? Could then make this just an assert. Minor point though, and would be fine as a follow-up.


================
Comment at: llvm/lib/Transforms/IPO/LowerTypeTests.cpp:297
 
+struct IcallJumptable final
+    : TrailingObjects<IcallJumptable, GlobalTypeMember *> {
----------------
nit: I would have capitalized `Icall` as `ICall` throughout.


https://reviews.llvm.org/D42453