This is an archive of the discontinued LLVM Phabricator instance.

[CallSiteInfo][CallGraphSection] Extend CallSiteInfo for indirect call type ids
AbandonedPublic

Authored by necipfazil on Jul 13 2021, 10:35 AM.

Details

Summary

Compute numeric type identifiers for indirect calls using generalized type
identifiers passed from the front-end, and carry them to the back-end with
CallSiteInfo.

The numeric type identifiers will be used at the back-end in the call graph
section.

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 created this revision.Jul 13 2021, 10:35 AM
necipfazil requested review of this revision.Jul 13 2021, 10:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2021, 10:35 AM

Has the YAML format of the callSites: (MF attribute) changed? If yes, can you please add a test case that demonstrates that?

Can we write a test?

llvm/include/llvm/CodeGen/MachineFunction.h
435

The cast looks unneeded.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
813

Nit: This variable can go closer to where it is used (or remove entirely).

llvm/lib/Target/X86/X86ISelLowering.cpp
3948

This pattern looks pretty similar across all the architectures. Can share more of this logic in CallSiteInfo?

Maybe a constructor that takes a CallBase * could do all of this.

lattner resigned from this revision.Jul 15 2021, 9:41 AM

Extract callsite type ids from operand bundles instead of from metadata

  • Parent commit now passes callsite type ids through operand bundles. Adapt type id extraction to the changes.
  • Fix a few nits
necipfazil marked 2 inline comments as done.Jul 16 2021, 12:49 PM
vitalybuka added a subscriber: vitalybuka.
vitalybuka added inline comments.
llvm/include/llvm/CodeGen/MachineFunction.h
419

Try to extract separate NFC patch for SmallVector<ArgRegPair, 1> -> struct CallSiteInfo

llvm/lib/CodeGen/MIRPrinter.cpp
522

You can avoid many of them with begin() end() implementation

vitalybuka added inline comments.Jul 16 2021, 2:18 PM
llvm/include/llvm/CodeGen/MachineFunction.h
419

to clarify, extractNumericCGTypeId stuff should be in a separate patch with some tests

morehouse added inline comments.Jul 19 2021, 12:44 PM
llvm/include/llvm/CodeGen/MachineFunction.h
444

cast already has an isa assertion, so we can remove those here.

https://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates

451
necipfazil marked 2 inline comments as done.

MipsISelLowering: Fill CallSiteInfo::ArgRegPairs only if -emit-call-site-info
ArgRegPairs was always filled but the container CallSiteInfo instance was only passed to DAG if -emit-call-site-info. Now we pass the CallSiteInfo instance to DAG when --call-graph-section as well. Avoid superfluous ArgRegPairs by filling them only when -emit-call-site-info.

Add typeId to callSites YAML format, add tests

  • MIRPrinter/MIRParser: Parse/print CallSiteInfo::TypeId. This also enables better testing of the feature.
  • Add tests: Test MIRPrinter/MIRParser. Test that call site type ids can be extracted and set from type operand bundles.
  • Fix nits.

Has the YAML format of the callSites: (MF attribute) changed? If yes, can you please add a test case that demonstrates that?

It is now changed with the latest updates (tests included).

necipfazil marked an inline comment as done.

Refactor extracting and setting type id
Create a constructor for CallSiteInfo that takes a CallBase instance to encapsulate extracting and setting type id, and any warnings/assertions.

morehouse added inline comments.Jul 23 2021, 10:34 AM
llvm/include/llvm/CodeGen/MachineFunction.h
425

This constructor is implicit from the above TypeId = nullptr. We can just remove this.

430

We can also rely on the default member initializer here.

llvm/lib/Target/X86/X86FastISel.cpp
3584–3586
necipfazil marked 5 inline comments as done.Jul 29 2021, 2:51 PM
necipfazil added inline comments.
llvm/include/llvm/CodeGen/MachineFunction.h
425

I kept this since the default constructor is used elsewhere, which is implicity deleted because of the below constructor CallSiteInfo(const CallBase).

Besides, I removed the unnecessary initializers for TypeId.

llvm/lib/CodeGen/MIRPrinter.cpp
522

I am not sure if I understand this. Could you please elaborate?

necipfazil marked an inline comment as done.Jul 29 2021, 2:53 PM

This patch is split to 4 per @vitalybuka's feedback: D107108, D107109, D107110, D107111.

The issues raised in this revision are fixed there. Also, a few, smaller tests are added.

We can now abandon this patch.

necipfazil abandoned this revision.Jul 29 2021, 3:05 PM