[PATCH] D65629: cfi-icall: Allow the jump table to be optionally made non-canonical.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc marked 6 inline comments as done.
pcc added inline comments.


================
Comment at: clang/test/CodeGen/cfi-icall-canonical-jump-tables.c:15
+// CHECK: define void @g({{.*}} [[ATTR2:#[0-9]+]]
+__attribute__((cfi_jump_table_canonical)) void g() {}
+
----------------
eugenis wrote:
> would it be more natural to spell it "cfi_canonical_jump_table" ?
> No strong feelings one way or the other.
Me neither. In the end it's probably a little better to be consistent with the flag name, so I've renamed it.


================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:328
+    if (lowertypetests::isJumpTableCanonical(&F))
       Linkage = CFL_Definition;
+    else if (F.hasExternalWeakLinkage())
----------------
eugenis wrote:
> Is this correct? Looks like it would cause function definitions with non-canonical jump tables to be exported as declarations. This could be desirable for LowerTypeTests, but could it affect other users negatively?
http://llvm-cs.pcc.me.uk/?q=%22cfi.functions%22
This metadata is only used by LowerTypeTests to decide how to create the jump table (and by CrossDSOCFI, but it doesn't look at this field). So I don't think it should affect anything else.


================
Comment at: llvm/test/Transforms/ThinLTOBitcodeWriter/cfi-functions-canonical-jump-tables.ll:4
+
+; CHECK: !"f1", i8 1
+; CHECK: !"f2", i8 1
----------------
eugenis wrote:
> Does this test check exported functions metadata? Could you add a check for the metadata name, or at least a comment to make this easier to understand in the future?
Yes, it's for exported functions. I've added a comment here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65629/new/

https://reviews.llvm.org/D65629