Page MenuHomePhabricator

Fix thinlto cache key computation for cfi-icall.
ClosedPublic

Authored by eugenis on Aug 4 2017, 7:07 PM.

Details

Summary

Fixes PR33966.

CFI code generation for users (not just callers) of a function depends
on whether this function has a jumptable entry or not. This
information needs to be encoded in thinlto cache key.

We filter the jumptable list against functions that are actually
referenced in the current module.

Diff Detail

Repository
rL LLVM

Event Timeline

eugenis created this revision.Aug 4 2017, 7:07 PM
eugenis edited the summary of this revision. (Show Details)Aug 4 2017, 7:08 PM
eugenis added reviewers: pcc, tejohnson, mehdi_amini.
eugenis set the repository for this revision to rL LLVM.
eugenis added a subscriber: llvm-commits.
pcc added inline comments.Aug 4 2017, 8:09 PM
llvm/lib/LTO/LTO.cpp
233 ↗(On Diff #109843)

We are recomputing this map (and the one below) once for each input file, which would seem to create O(NM) complexity. Can we avoid that by moving the map computation into the InProcessThinBackend? See for example how we handle TypeIdSummariesByGuid.

256 ↗(On Diff #109843)

I think you can move the body of this loop into AddTypeIdSummary (after giving it a better name).

258 ↗(On Diff #109843)

I think you also need to include calls().

llvm/test/ThinLTO/X86/Inputs/cache-icall-no.ll
1 ↗(On Diff #109843)

Do you need this file as part of your test case? I think you just need to *not* provide an LTO definition of f. That's basically what the reproducer in the PR is doing.

eugenis updated this revision to Diff 110092.Aug 7 2017, 2:31 PM

Addressed pcc's comments.

eugenis marked 4 inline comments as done.Aug 7 2017, 2:31 PM
pcc accepted this revision.Aug 7 2017, 2:57 PM

LGTM

llvm/lib/LTO/LTO.cpp
179 ↗(On Diff #110092)

So GS can't be null here? If so, you can use dyn_cast below.

867 ↗(On Diff #110092)

I think you need to call GlobalValue::dropLLVMManglingEscape on the function name here (same below).

This revision is now accepted and ready to land.Aug 7 2017, 2:57 PM
eugenis updated this revision to Diff 110099.Aug 7 2017, 3:07 PM

add a null check && dropLLVMManglingEscape

eugenis marked an inline comment as done.Aug 7 2017, 3:07 PM
eugenis added inline comments.
llvm/lib/LTO/LTO.cpp
179 ↗(On Diff #110092)

in fact, it can (but not in the test suite)

This revision was automatically updated to reflect the committed changes.