Skip to content

[MemProf][PGO] Prevent dropping of profile metadata during optimization #121359

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 2, 2025
Merged

[MemProf][PGO] Prevent dropping of profile metadata during optimization #121359

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3e226cb
[MemProf][PGO] Prevent dropping of profile metadata during optimization
teresajohnson Dec 30, 2024
133e8d7
Add FIXME comments with reference to new issue, and invoke newly created
teresajohnson Jan 2, 2025
11ac81d
Simplify merging code
teresajohnson Jan 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,10 +345,11 @@ static bool writtenBetween(MemorySSA *MSSA, BatchAAResults &AA,
static void combineAAMetadata(Instruction *ReplInst, Instruction *I) {
// FIXME: MD_tbaa_struct and MD_mem_parallel_loop_access should also be
// handled here, but combineMetadata doesn't support them yet
unsigned KnownIDs[] = {LLVMContext::MD_tbaa, LLVMContext::MD_alias_scope,
LLVMContext::MD_noalias,
LLVMContext::MD_invariant_group,
LLVMContext::MD_access_group};
unsigned KnownIDs[] = {
LLVMContext::MD_tbaa, LLVMContext::MD_alias_scope,
LLVMContext::MD_noalias, LLVMContext::MD_invariant_group,
LLVMContext::MD_access_group, LLVMContext::MD_prof,
LLVMContext::MD_memprof, LLVMContext::MD_callsite};
combineMetadata(ReplInst, I, KnownIDs, true);
}

Expand Down
8 changes: 7 additions & 1 deletion llvm/lib/Transforms/Utils/Local.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3379,6 +3379,10 @@ void llvm::combineMetadata(Instruction *K, const Instruction *J,
K->setMetadata(Kind,
MDNode::getMostGenericAlignmentOrDereferenceable(JMD, KMD));
break;
case LLVMContext::MD_memprof:
case LLVMContext::MD_callsite:
// Preserve !memprof and !callsite metadata on K.
Copy link
Contributor

@nikic nikic Dec 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be merging the contents of the memprof and callsite metadata, rather than taking it from one of the instructions?

Copy link
Contributor Author

@teresajohnson teresajohnson Dec 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't a great way to merge them at the moment, unfortunately. They encode information about the call context and it can't currently support multiple distinct call locations. For now we pick one. In the actual case I was looking at in real code this handling was invoked by the byval argument optimization in MemCpyOptimizer though, where we aren't even combining two calls, so this change just ensures it isn't dropped unnecessarily (as you note this probably isn't the best facility to use in that code anyway).

Copy link
Contributor

@nikic nikic Jan 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's not possible to correctly merge the metadata, we shouldn't include it in combineMetadata, as it will produce an incorrect result for the sink/hoist cases. It sounds like it would be sufficient for your case to only fix what MemCpyOptimizer does (to avoid merging for memprof/callsite in the first place). I'd rather not introduce incorrect merging logic in combineMetadata to cancel out incorrect code in MemCpyOptimizer...

Copy link
Contributor Author

@teresajohnson teresajohnson Jan 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't incorrect to keep one. Since it is profile metadata, it doesn't affect correctness. We can improve how this is combined in the future, but simply keeping one set for now is a reasonable heuristic.

Copy link
Contributor Author

@teresajohnson teresajohnson Jan 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't incorrect to keep one. Since it is profile metadata, it doesn't affect correctness. We can improve how this is combined in the future, but simply keeping one set for now is a reasonable heuristic.

To that end, it might be better to add methods to merge the memprof related metadata, as is done for some of the other metadata types (e.g. getMergedProfMetadata), so we can keep that handling centralized with other memprof code and add TODOs for more sophisticated merging. I will do that.

break;
case LLVMContext::MD_preserve_access_index:
// Preserve !preserve.access.index in K.
break;
Expand Down Expand Up @@ -3442,7 +3446,9 @@ void llvm::combineMetadataForCSE(Instruction *K, const Instruction *J,
LLVMContext::MD_nontemporal,
LLVMContext::MD_noundef,
LLVMContext::MD_mmra,
LLVMContext::MD_noalias_addrspace};
LLVMContext::MD_noalias_addrspace,
LLVMContext::MD_memprof,
LLVMContext::MD_callsite};
combineMetadata(K, J, KnownIDs, KDominatesJ);
}

Expand Down
18 changes: 18 additions & 0 deletions llvm/test/Transforms/MemCpyOpt/memcpy.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -803,6 +803,19 @@ define void @byval_param_noalias_metadata(ptr align 4 byval(i32) %ptr) {
ret void
}

define void @byval_param_profile_metadata(ptr align 4 byval(i32) %ptr) {
; CHECK-LABEL: @byval_param_profile_metadata(
; CHECK-NEXT: store i32 1, ptr [[PTR2:%.*]], align 4
; CHECK-NEXT: call void @f_byval(ptr byval(i32) align 4 [[PTR2]]), !prof [[PROF3:![0-9]+]], !memprof [[META4:![0-9]+]], !callsite [[META7:![0-9]+]]
; CHECK-NEXT: ret void
;
%tmp = alloca i32, align 4
store i32 1, ptr %ptr
call void @llvm.memcpy.p0.p0.i64(ptr align 4 %tmp, ptr align 4 %ptr, i64 4, i1 false)
call void @f_byval(ptr align 4 byval(i32) %tmp), !memprof !3, !callsite !6, !prof !7
ret void
}

define void @memcpy_memory_none(ptr %p, ptr %p2, i64 %size) {
; CHECK-LABEL: @memcpy_memory_none(
; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i64(ptr [[P:%.*]], ptr [[P2:%.*]], i64 [[SIZE:%.*]], i1 false) #[[ATTR7:[0-9]+]]
Expand Down Expand Up @@ -897,3 +910,8 @@ define void @memcpy_immut_escape_after(ptr align 4 noalias %val) {
!0 = !{!0}
!1 = !{!1, !0}
!2 = !{!1}
!3 = !{!4}
!4 = !{!5, !"cold"}
!5 = !{i64 123, i64 456}
!6 = !{i64 123}
!7 = !{!"branch_weights", i32 10}
51 changes: 51 additions & 0 deletions llvm/test/Transforms/SimplifyCFG/merge-calls-memprof.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5

;; Test to ensure that memprof related metadata is not dropped when
;; instructions are combined. Currently the metadata from the first instruction
;; is kept, which prevents full loss of profile context information.

; RUN: opt < %s -passes=simplifycfg -S | FileCheck %s

target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

define dso_local noundef nonnull ptr @_Z4testb(i1 noundef zeroext %b) local_unnamed_addr #0 {
; CHECK-LABEL: define dso_local noundef nonnull ptr @_Z4testb(
; CHECK-SAME: i1 noundef zeroext [[B:%.*]]) local_unnamed_addr {
; CHECK-NEXT: [[ENTRY:.*:]]
; CHECK-NEXT: [[CALL:%.*]] = call noalias noundef nonnull dereferenceable(4) ptr @_Znwm(i64 noundef 4), !memprof [[META0:![0-9]+]], !callsite [[META3:![0-9]+]]
; CHECK-NEXT: ret ptr [[CALL]]
;
entry:
br i1 %b, label %if.then, label %if.else

if.then: ; preds = %entry
%call = call noalias noundef nonnull dereferenceable(4) ptr @_Znwm(i64 noundef 4), !memprof !0, !callsite !3
br label %if.end

if.else: ; preds = %entry
%call1 = call noalias noundef nonnull dereferenceable(4) ptr @_Znwm(i64 noundef 4), !memprof !4, !callsite !7
br label %if.end

if.end: ; preds = %if.else, %if.then
%x.0 = phi ptr [ %call, %if.then ], [ %call1, %if.else ]
ret ptr %x.0
}


declare ptr @_Znwm(i64) nounwind readonly

!0 = !{!1}
!1 = !{!2, !"notcold"}
!2 = !{i64 -852997907418798798, i64 -2101080423462424381, i64 5188446645037944434}
!3 = !{i64 -852997907418798798}
!4 = !{!5}
!5 = !{!6, !"cold"}
!6 = !{i64 123, i64 -2101080423462424381, i64 5188446645037944434}
!7 = !{i64 123}
;.
; CHECK: [[META0]] = !{[[META1:![0-9]+]]}
; CHECK: [[META1]] = !{[[META2:![0-9]+]], !"notcold"}
; CHECK: [[META2]] = !{i64 -852997907418798798, i64 -2101080423462424381, i64 5188446645037944434}
; CHECK: [[META3]] = !{i64 -852997907418798798}
;.
Loading