This is an archive of the discontinued LLVM Phabricator instance.

IR: Function summary representation for type tests.
ClosedPublic

Authored by pcc on Dec 19 2016, 6:59 PM.

Details

Summary

Each function summary has an attached list of type identifier GUIDs. The
idea is that during the regular LTO phase we would match these GUIDs to type
identifiers defined by the regular LTO module and store the resolutions in
a top-level "type identifier summary" (which will be implemented separately).

Depends on D27875

Event Timeline

pcc updated this revision to Diff 82057.Dec 19 2016, 6:59 PM
pcc retitled this revision from to IR: Function summary representation for type tests..
pcc updated this object.
pcc added reviewers: tejohnson, mehdi_amini.
pcc added a subscriber: llvm-commits.
pcc updated this revision to Diff 82153.Dec 20 2016, 2:11 PM

Refresh

mehdi_amini added inline comments.Dec 20 2016, 5:13 PM
llvm/lib/Analysis/ModuleSummaryAnalysis.cpp
128

The comment is outdated.

134

Is 1 the common expectation?

137

So we don't care about DevirtCalls and about Assumes but we're still gonna populate them?

Why extending this API and calling it when it seems that what you want is simply:

bool HasNonAssumeUses = llvm::any_of(CI->uses(), [] (const Use &CIU) {
    return !isa<CallInst>(CIU.getUser());
}

Even though it should likely rather be:

bool HasNonAssumeUses = llvm::any_of(CI->uses(), [] (const Use &CIU) {
    if (auto *AssumeCI = dyn_cast<CallInst>(CIU.getUser())) {
      Function *F = AssumeCI->getCalledFunction();
      if (F && F->getIntrinsicID() == Intrinsic::assume) 
        return false;
    }
    return true;
}
138

A comment about why only adding type test that only have an assume as user would be welcome for the reader here.

145–146

Just a nit, you can do early continue here:

if (CalledFunction->getIntrinsicID() != Intrinsic::type_test)
    continue;
...
llvm/lib/Analysis/TypeMetadataUtils.cpp
75

Unrelated but looks like it could be if (auto *AssumeCI = dyn_cast<CallInst>(CIU.getUser()))

78–80

Technically, the else path here should also set HasNonAssumeUses = true

llvm/lib/Bitcode/Reader/BitcodeReader.cpp
5047

Can we assert PendingTypeTests.empty() here?

llvm/test/Bitcode/thinlto-type-tests.ll
31

Can you add a test with a type.test without any user, and another with a type.test that only has an assume as user?
(Since you special case these).

pcc updated this revision to Diff 82258.Dec 21 2016, 12:55 PM
pcc marked 6 inline comments as done.
  • Address review comments
llvm/lib/Analysis/ModuleSummaryAnalysis.cpp
134

Yes, basically the pattern created by the frontend is: @llvm.assert(@llvm.type.test(...)). I think it could only be different if GVN or some other pass managed to combine two calls to @llvm.type.test. I've only seen that happen once or twice in chromium though.

137

Works for me, I was getting a little ahead of myself to when we'll need to look at the calls for devirtualization. But that can all happen later.

llvm/lib/Analysis/TypeMetadataUtils.cpp
75

r290264

78–80

I ended up reverting this part.

llvm/test/Bitcode/thinlto-type-tests.ll
31

llvm/test/Bitcode/thinlto-unused-type-tests.ll

mehdi_amini accepted this revision.Dec 21 2016, 3:06 PM
mehdi_amini edited edge metadata.

LGTM.

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp
151

Thanks for the comment here.

This revision is now accepted and ready to land.Dec 21 2016, 3:06 PM
This revision was automatically updated to reflect the committed changes.