[PATCH] D42611: [ThinLTO/CFI] Include TYPE_ID summaries into GLOBALVAL_SUMMARY_BLOCK

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: clang/test/CodeGen/thinlto-distributed-cfi-devirt.ll:39-43
+; CHECK-IR-LABEL: define i32 @main
+; CHECK-IR: br i1 {{.*}}, label %trap
+; CHECK-IR: br i1 {{.*}}, label %trap
+; CHECK-IR-LABEL: ret i32
+; CHECK-IR-LABEL: }
----------------
I would move these alongside the IR that is being tested.


================
Comment at: clang/test/CodeGen/thinlto-distributed-cfi-devirt.ll:58-59
+ at _ZTV1B = constant { [3 x i8*] } { [3 x i8*] [i8* null, i8* null, i8* bitcast (i32 (%struct.B*, double)* @_ZN1B1fEd to i8*)] }, align 8, !type !0, !type !1
+ at _ZTI1A = constant { i8*, i8* } { i8* null, i8* null }
+ at _ZTI1B = constant { i8*, i8*, i8* } { i8* null, i8* null, i8* null }
+ at _ZTV1C = constant { [3 x i8*] } { [3 x i8*] [i8* null, i8* null, i8* bitcast (i32 (%struct.C*, double)* @_ZN1C1fEd to i8*)] }, align 8, !type !0, !type !2
----------------
Remove these.


================
Comment at: clang/test/CodeGen/thinlto-distributed-cfi-devirt.ll:69
+  %2 = bitcast i8* %call1 to %struct.A*
+  %3 = tail call { i8*, i1 } @llvm.type.checked.load(i8* bitcast (i8** getelementptr inbounds ({ [3 x i8*] }, { [3 x i8*] }* @_ZTV1B, i64 0, inrange i32 0, i64 2) to i8*), i32 0, metadata !"_ZTS1A")
+  %4 = extractvalue { i8*, i1 } %3, 1
----------------
This intrinsic call looks a little odd if the intent is to test that whole-program devirtualization was not applied. In principle it could be locally devirtualized by loading a function pointer from the constant `_ZTV1B`, which would make the branch below unconditional. Can you instead load a vtable from a passed-in object and pass that to the intrinsic? Basically, make this look like `call2` or `call3` in `llvm/test/Transforms/WholeProgramDevirt/import.ll`. The same goes for the other call below.

It was also a little surprising to me at first that this wasn't virtual-const-propagated, but I guess the reason why it wasn't is that you are passing an argument of type `double`. A more reliable way of preventing virtual const prop would be to pass `undef` as one of the arguments (see `call2` in the same test).


================
Comment at: clang/test/CodeGen/thinlto-distributed-cfi-devirt.ll:82
+  %call2 = tail call i32 %7(%struct.A* nonnull %5, double 0.0)
+  %8 = tail call { i8*, i1 } @llvm.type.checked.load(i8* bitcast (i8** getelementptr inbounds ({ [3 x i8*] }, { [3 x i8*] }* @_ZTV1C, i64 0, inrange i32 0, i64 2) to i8*), i32 0, metadata !"_ZTS1A")
+  %9 = extractvalue { i8*, i1 } %8, 1
----------------
Can you please also include testing for successful devirtualization? I guess you could do that by changing the metadata here to `!"_ZTS1B"` and testing that the call was successfully single-impl-devirtualized.


================
Comment at: clang/test/CodeGen/thinlto-distributed-cfi.ll:27
+; RUN:   -emit-obj -fthinlto-index=%t.o.thinlto.bc \
+; RUN:   -emit-llvm -o %t.ll -x ir %t.o
+; RUN: FileCheck --input-file %t.ll %s --check-prefixes=CHECK-IR
----------------
You could write `-o -` here and pipe into `FileCheck` as usual.


================
Comment at: clang/test/CodeGen/thinlto-distributed-cfi.ll:49
+entry:
+  %0 = tail call i1 @llvm.type.test(i8* bitcast (i8** getelementptr inbounds ({ [3 x i8*] }, { [3 x i8*] }* @_ZTV1B, i64 0, inrange i32 0, i64 2) to i8*), metadata !"_ZTS1A")
+  br i1 %0, label %cont, label %trap
----------------
The backend could in principle replace this with `true` because the first argument is a constant. Could you instead add an argument to `main` and pass that argument through as the first argument here?


https://reviews.llvm.org/D42611