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

[Evgenii Stepanov via Phabricator via llvm-commits]

eugenis 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() {}
+
----------------
would it be more natural to spell it "cfi_canonical_jump_table" ?
No strong feelings one way or the other.


================
Comment at: clang/test/SemaCXX/attr-cfi-jump-table-canonical.cpp:3
+
+__attribute__((cfi_jump_table_canonical)) void f() {}
+
----------------
Test the new attribute on function declaration.


================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:328
+    if (lowertypetests::isJumpTableCanonical(&F))
       Linkage = CFL_Definition;
+    else if (F.hasExternalWeakLinkage())
----------------
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?


================
Comment at: llvm/test/Transforms/ThinLTOBitcodeWriter/cfi-functions-canonical-jump-tables.ll:4
+
+; CHECK: !"f1", i8 1
+; CHECK: !"f2", i8 1
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65629