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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
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;
----------------
chandlerc wrote:
> 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).
Sorry, that was one of the things that I forgot to update in the code. Renamed to "branch funnel".


================
Comment at: llvm/include/llvm/Target/Target.td:1146
+  let InOperandList = (ins variable_ops);
+  let AsmString = "";
+  let hasSideEffects = 1;
----------------
chandlerc wrote:
> 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.)
No, the MI dump already says "ICALL_BRANCH_FUNNEL". This string is used in the asm output for instructions that survive until MC, and this one doesn't.


================
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");
+
----------------
chandlerc wrote:
> 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.
This particular check can't be a verifier check because the wholeprogramdevirt pass creates IR that would not pass this check (it is later fixed up by lowertypetests). The others would be fine as verifier checks and we might do that as a followup.


https://reviews.llvm.org/D42453