Page MenuHomePhabricator

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

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



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

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

Original RFC:
Updated RFC:

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?


The cast looks unneeded.


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


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.

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


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

vitalybuka added inline comments.Jul 16 2021, 2:18 PM

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

morehouse added inline comments.Jul 19 2021, 12:44 PM

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

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

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


We can also rely on the default member initializer here.

necipfazil marked 5 inline comments as done.Jul 29 2021, 2:51 PM
necipfazil added inline comments.

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.


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