[PATCH] D128141: [MemProf] Basic metadata support and verification

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/IR/Verifier.cpp:4536
+
+void Verifier::visitCallsiteMetadata(Instruction &I, MDNode *MD) {
+  visitCallStackMetadata(MD);
----------------
snehasish wrote:
> Instruction& I is unused. 
> 
> Perhaps add a check here that I is a CallInst? If you do then also add a test for it.
Good idea, added this check both here and in visitMemProfMetadata, and added checks for both in the bad metadata test.


================
Comment at: llvm/test/Verifier/memprof-metadata-good.ll:29
+!2 = !{i64 123, i64 456}
+!3 = !{!4, !"cold", !"tag3", !"tag4"}
+!4 = !{i64 123, i64 789, i64 678}
----------------
Enna1 wrote:
> IIUC, does this !4 = !{i64 123, i64 789, i64 678} means we have a call stack: test3() -> test1() -> malloc()
> If so, the callee in `test3()` should be `test1()` instead of `test2()` ? :
> ```
> define i32* @test3() {
> entry:
>   %call = call noundef i32* @test1(), !callsite !7
>   ret i32* %call
> }
> ```
Indeed! This is a good catch, mistake in my hand crafted test case IR. Fixed. (Although it doesn't matter for verification and in general we can't detect this kind of issue since the call chains can cross module boundaries.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128141/new/

https://reviews.llvm.org/D128141