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

[Dmitry Mikulin via Phabricator via llvm-commits]

dmikulin marked an inline comment as done.
dmikulin added inline comments.


================
Comment at: llvm/lib/IR/Value.cpp:506
+      continue;
+    U.set(New);
+  }
----------------
vlad.tsyrklevich wrote:
> 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)`.
I can't seem to reproduce it without at least having the fptr argument wrapped in a bitcast, which makes this check work. I'm thinking I'd need a recursively defined fptr type to avoid the cast. 
I don't necessarily like the roundabout way of checking if it's a direct call. Is there a better way?


================
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
+
----------------
vlad.tsyrklevich wrote:
> 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.
I'll work on the test cases.


================
Comment at: llvm/test/Transforms/LowerTypeTests/cfi-direct-call.ll:6
+
+declare void @external()
+
----------------
vlad.tsyrklevich wrote:
> 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.)
Direct calls to external functions may still go through a jump table entry, if those functions are of the same type as functions called indirectly. I'll change the test case and add more test points.


https://reviews.llvm.org/D46326