This is an archive of the discontinued LLVM Phabricator instance.

[clang][CallGraphSection] Add type id metadata to indirect call and targets
AcceptedPublic

Authored by Prabhuk on Jul 13 2021, 10:22 AM.

Details

Summary

Create and add generalized type identifier metadata to indirect calls,
and to functions that may be target to indirect calls.

Type identifiers will be used by the back-end to construct the call
graph section to precisely represent the possible targets for indirect calls.
The type information is deliberately pulled from the front-end to have extra
precision since some type information is lost at IR, and to ensure
consistent type identifiers between object files compiled at different
times (as C/C++ standards require language-level types to match).

Original RFC: https://lists.llvm.org/pipermail/llvm-dev/2021-June/151044.html
Updated RFC: https://lists.llvm.org/pipermail/llvm-dev/2021-July/151739.html

Diff Detail

Event Timeline

necipfazil requested review of this revision.Jul 13 2021, 10:22 AM
necipfazil created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2021, 10:22 AM

I think we want a C++ test as well, with class functions, templates, etc.

Also, please rebase the patch onto any parent commits, so that it builds properly and the clang-tidy warnings go away.

clang/lib/CodeGen/CGObjCMac.cpp
2288 ↗(On Diff #358327)

This is for Objective C? I don't think we care about Objective C.

clang/lib/CodeGen/CodeGenModule.cpp
1878 ↗(On Diff #358327)

Why do we need the type metadata on function definitions?

clang/test/CodeGen/call-graph-section.c
4–7 ↗(On Diff #358327)
9 ↗(On Diff #358327)

Nit: I find this spacing more readable

13 ↗(On Diff #358327)

This recaptures the TVOID_GENERALIZED variable. We should reuse the previous one instead: [[TVOID_GENERALIZED]].

Same for other variables below.

39 ↗(On Diff #358327)

Should we also add a test with a C struct parameter?

41 ↗(On Diff #358327)

What is the {{.*}} consuming here?

necipfazil added inline comments.Jul 13 2021, 8:54 PM
clang/lib/CodeGen/CGObjCMac.cpp
2288 ↗(On Diff #358327)

I will remove this.

clang/lib/CodeGen/CodeGenModule.cpp
1878 ↗(On Diff #358327)

Actually, we don’t. It seems like having this may lead to duplicate metadata as well.

I will remove this.

clang/test/CodeGen/call-graph-section.c
39 ↗(On Diff #358327)

I will add one.

41 ↗(On Diff #358327)

Internally, for functions, this metadata is added using llvm::GlobalObject::addTypeMetadata(), which takes an offset parameter. This is always 0 for our case. {{.*}} is consuming i64 0, if the metadata belongs to a function. Otherwise, for callsites, it consumes nothing.

I will make the distinction in the test case.

Several review comments are addressed

  • Don't emit type metadata for Objective-C
  • Don't emit metadata for function definitions, which leads to duplicates
  • Improve the test case
    • Add C struct parameter test
    • Fix variable capturing
    • Make function and callsite metadata checking distinct
    • Some formatting

TODO: a C++ test will be added with class functions, templates, etc.

morehouse added inline comments.Jul 14 2021, 1:46 PM
clang/lib/CodeGen/CodeGenModule.cpp
2068 ↗(On Diff #358505)

This code also seems unnecessary as it puts metadata on function definitions.

2177 ↗(On Diff #358505)

Also seems unnecessary.

clang/test/CodeGen/call-graph-section.c
20 ↗(On Diff #358505)

Do we still expect type metadata on function definitions on the latest diff?

39 ↗(On Diff #358327)

I mean an indirect call with a type like this:

void foo(struct MyStruct s)

Currently all the tests use primitive types.

Also, have you looked into operand bundles instead of metadata? I don't know much about them, but they might help with the optimizations-dropping-metadata problem.

necipfazil added inline comments.Jul 15 2021, 8:43 AM
clang/lib/CodeGen/CodeGenModule.cpp
2068 ↗(On Diff #358505)

We need the metadata for mapping indirect calls to receiver indirect targets with matching type id.

2177 ↗(On Diff #358505)

As described above, we need the metadata for indirect targets.

clang/test/CodeGen/call-graph-section.c
20 ↗(On Diff #358505)

As described above, we need the metadata for indirect targets.

  • rebase
  • add C struct parameter test
  • add C++ tests

Use operand bundles for callsite type ids

  • Use (unlossy) operand bundles for propagating indirect callsite type ids
  • No longer use type metadata for indirect callsites
  • Adapt the tests to the changes

Currently, all callsites are annotated with a type operand bundle regardless
of whether indirect or not. While this does not affect the correctness for
call graph section as the extra information is not used, unnecessary operand
bundles on direct callsites increases IR program size. This will be fixed.

Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2021, 11:43 AM

Also, have you looked into operand bundles instead of metadata? I don't know much about them, but they might help with the optimizations-dropping-metadata problem.

Latest changes switches to using operand bundles instead of metadata. Besides solving the optimizations-dropping-metadata problem, it simplified the code as well.

necipfazil marked 10 inline comments as done.Jul 17 2021, 9:07 PM
morehouse added inline comments.Jul 19 2021, 11:47 AM
clang/lib/CodeGen/CGCall.cpp
5255 ↗(On Diff #359401)

Please fix this lint.

llvm/include/llvm/IR/LLVMContext.h
97 ↗(On Diff #359401)

Do we need to update LLVMContext::LLVMContext()?

necipfazil marked 2 inline comments as done.

Fix lint

llvm/include/llvm/IR/LLVMContext.h
97 ↗(On Diff #359401)

Operand bundles are originally used as input to lowering in SelectionDAGBuilder. Arbitrary operand bundles are not allowed.

To allow type operand bundle, we need this change. Please see llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp for related changes.

morehouse added inline comments.Jul 22 2021, 10:43 AM
llvm/include/llvm/IR/LLVMContext.h
97 ↗(On Diff #359401)

So why don't you need to update LLVMContext::LLVMContext() then? All other operand bundle types are there.

Verify type operand bundle usage

  • Check if type operand bundle id is drifted
  • Verify and complain if multiple type operand bundles are used
  • Add type operand bundle test to operand bundle verifier tests
necipfazil marked an inline comment as done.Jul 22 2021, 1:06 PM
necipfazil added inline comments.
llvm/include/llvm/IR/LLVMContext.h
97 ↗(On Diff #359401)

I added further verifications in LLVMContext::LLVMContext() and some tests for it.

If we requested updates are for somewhere else, could you please provide more details?

morehouse accepted this revision.Jul 22 2021, 1:54 PM
morehouse added inline comments.
llvm/test/Verifier/operand-bundles.ll
91 ↗(On Diff #360942)
This revision is now accepted and ready to land.Jul 22 2021, 1:54 PM
necipfazil marked 2 inline comments as done.

Fix nit

(Two comments I forgot to push yesterday.)

clang/test/CodeGen/call-graph-section-1.cpp
39 ↗(On Diff #359401)

needs a test for an out-of-line definition.

clang/test/CodeGen/call-graph-section-3.cpp
24 ↗(On Diff #362652)

Metadata nodes are guaranteed to be in the end. Drop -DAG

Herald added a project: Restricted Project. · View Herald TranscriptWed, Nov 22, 1:53 PM
Prabhuk updated this revision to Diff 558152.Wed, Nov 22, 2:11 PM

Rebased the patchset and addressed the compilation and test failures

phosek added a subscriber: phosek.Mon, Nov 27, 9:53 AM