[PATCH] D46326: ThinLTO+CFI: short-circuit direct calls to jump table entries

[Vlad Tsyrklevich via Phabricator via llvm-commits]

vlad.tsyrklevich added a comment.

It would be nice to also add a direct call monolithic LTO test as monolithic LTO has a slightly different code path for renaming functions and based on a quick look above it looks like we might only correctly rename direct calls to local, not external, functions from the code above.



================
Comment at: llvm/include/llvm/IR/Value.h:307
+  /// uses.
+  void replaceCfiUses(Value *New);
+
----------------
Given that this function usefulness is now likely specific to LowerTypeTests it's probably worth just including it there directly instead of polluting all of Value's children.


================
Comment at: llvm/lib/IR/Value.cpp:506
+      continue;
+    U.set(New);
+  }
----------------
dmikulin wrote:
> vlad.tsyrklevich wrote:
> > dmikulin wrote:
> > > efriedma wrote:
> > > > This looks like it doesn't handle the case of a function with a function pointer argument correctly.
> > > I thought getCalledFunction on a CallInst takes care of it... 
> > It checks that the call is a direct call, but the use might be an argument of the function not the called function, e.g.
> > ```
> > void bar();
> > foo(&bar);
> > ```
> > 
> > Iterating through the uses of `bar` you'll hit the call instruction and replace `foo(&bar)` with `foo(&bar.cfi)` though it shouldn't be. You need to check that getCalledFunction is the one you actually expect.
> I see... I thought there'd be a more straightforward direct call check, but I can't seem to find anything in the code. Changed to check if the getCalledFunction() is the same as what we're trying to replace, i.e. ''this'.
I haven't tested it but I think this will not work for `foo(&foo)`, e.g. it will change it to `foo.cfi(&foo.cfi)` instead of `foo.cfi(&foo)`.


================
Comment at: llvm/test/Transforms/LowerTypeTests/cfi-direct-call.ll:1
+; RUN: opt -lowertypetests -lowertypetests-summary-action=import -lowertypetests-read-summary=%p/Inputs/cfi-direct-call.yaml %s -S | FileCheck %s
+
----------------
I'd like to see this test test the correct direct call behavior for 3 types of functions (matching the 3 cases in ::importFunction): local function declared in the current translation unit, local function declared in another TU, and an external function.


================
Comment at: llvm/test/Transforms/LowerTypeTests/cfi-direct-call.ll:6
+
+declare void @external()
+
----------------
This variable is named `@external` but is included in `CfiFunctionDefs` instead of `CfiFunctionsDecls` in the summary so it's treated like a local function. e.g. for direct calls it's re-written to `@external.cfi`. An direct call to an external function would be left as `@external` (instead of being re-written to `@external.cfi_jt` to point to the jump table entry.)


================
Comment at: llvm/test/Transforms/LowerTypeTests/import-icall.ll:31
 ; CHECK:      define hidden i8 @local_a.cfi() {
-; CHECK-NEXT:   call void @external.cfi_jt()
+; CHECK-NEXT:   call void @external()
 ; CHECK-NEXT:   call void select (i1 icmp ne (void ()* @external_weak, void ()* null), void ()* @external_weak.cfi_jt, void ()* null)()
----------------
For @external and @local_a below we should add an instruction where both address-taken so that we can still test the original behavior (e.g. that non-direct calls point to the jump table aliases as expected.)


https://reviews.llvm.org/D46326