[PATCH] D44399: [ThinLTO] Add funtions in callees metadata to CallGraphEdges

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D44399#1035340, @twoh wrote:

> @tejohnson I think your right. What I meant was that when the metadata is imported to `bar.o`, it references `f1` and `f2` by their promoted names, which makes the declarations with the promoted names to be added. Did I get it right, or still miss something?


Sort of - we import the references to f1 and f2 (refenced on the callee metadata for the imported function), and ThinLTO knows that any references we import that were previously local must be promoted. This usually works fine because when we export a function (foo in this case), we walk the edges and mark anything it references as exported, which means they will be promoted in the original module (foo.o here). The issue is that there were no reference or call edges created for the references in the callee metadata, so we didn't know those references were exported.

> For your second question, I assumed that the logic following the patch (line 304-311) will update the hotness info if available.

No, that is specific to indirect call PGO (attached as value profile metadata), which we don't have in this case (although I suppose if PGO is used we could end up with a callees metadata and VP metadata??). The VP metadata lists the count for each profiled target. Here we don't have anything, so I'm not sure what to suggest. The two possibilities would be to divide the instructions scaled count (computed in preceding code for the direct call case), and divide it evenly between the callee targets (so each would get 50% of that in your example). The other possibility, that requires more changes, but is potentially more accurate, is to extend the callees metadata and put some probability data on that based on the hotness of the block that assigned the function pointer to that callee. That's probably overkill at this point, so perhaps either leave as Unknown, or the first strategy I suggested.

Ok to do later, but you should put a FIXME noting that we are not setting any hotness for now.



================
Comment at: test/ThinLTO/X86/callees-metadata.ll:10
+; RUN: llvm-dis %t.o.1.3.import.bc -o - | FileCheck %s
+; CHECK: define {{.*}} i32 @f1.llvm.0
+
----------------
Add comment that we are testing to make sure that callees metadata functions are imported. 

Check for f2.llvm.0 also?


Repository:
  rL LLVM

https://reviews.llvm.org/D44399