[MemProf][PGO] Prevent dropping of profile metadata during optimization #121359
Conversation
This patch fixes a couple of places where memprof-related metadata (!memprof and !callsite) were being dropped, and one place where PGO metadata (!prof) was being dropped. All were due to instances of combineMetadata() being invoked. That function drops all metadata not in the list provided by the client, and also drops any not in its switch statement. Memprof metadata needed a case in the combineMetadata switch statement. For now we simply keep the metadata of the instruction being kept, which doesn't retain all the profile information when two calls with memprof metadata are being combined, but at least retains some. For the memprof metadata being dropped during call CSE, add memprof and callsite metadata to the list of known ids in combineMetadataForCSE. Neither memprof nor regular prof metadata were in the list of known ids for the callsite in MemCpyOptimizer, which was added to combine AA metadata after optimization of byval arguments fed by memcpy instructions, and similar types of optimizations of memcpy uses. There is one other callsite of combineMetadata, but it is only invoked on load instructions, which do not carry these types of metadata.
|
@llvm/pr-subscribers-llvm-analysis @llvm/pr-subscribers-llvm-transforms Author: Teresa Johnson (teresajohnson) ChangesThis patch fixes a couple of places where memprof-related metadata All were due to instances of combineMetadata() being invoked. That Memprof metadata needed a case in the combineMetadata switch statement. For the memprof metadata being dropped during call CSE, add memprof and Neither memprof nor regular prof metadata were in the list of known ids There is one other callsite of combineMetadata, but it is only invoked Full diff: https://github.com/llvm/llvm-project/pull/121359.diff 4 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp b/llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
index bb98b3d1c07259..c04258da912bb2 100644
--- a/llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
+++ b/llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
@@ -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);
}
diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp
index a3af96d5af026d..ecea00a55f0c51 100644
--- a/llvm/lib/Transforms/Utils/Local.cpp
+++ b/llvm/lib/Transforms/Utils/Local.cpp
@@ -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.
+ break;
case LLVMContext::MD_preserve_access_index:
// Preserve !preserve.access.index in K.
break;
@@ -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);
}
diff --git a/llvm/test/Transforms/MemCpyOpt/memcpy.ll b/llvm/test/Transforms/MemCpyOpt/memcpy.ll
index 39b90adc74ef38..65d78f4199aa02 100644
--- a/llvm/test/Transforms/MemCpyOpt/memcpy.ll
+++ b/llvm/test/Transforms/MemCpyOpt/memcpy.ll
@@ -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]+]]
@@ -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}
diff --git a/llvm/test/Transforms/SimplifyCFG/merge-calls-memprof.ll b/llvm/test/Transforms/SimplifyCFG/merge-calls-memprof.ll
new file mode 100644
index 00000000000000..10c6aeb26ba767
--- /dev/null
+++ b/llvm/test/Transforms/SimplifyCFG/merge-calls-memprof.ll
@@ -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}
+;.
|
|
@nikic curious for your thoughts on combineMetadata - it seems really fragile to new types of metadata, or new invocations such as in the case of the MemCpyOpt callsite, making it very easy to silently lose metadata (even !prof metadata was affected in one case I saw in real code). Is there a better way to do this? |
|
@DianQK adding you as it looks like you had added the calls to combineMetadata from MemCpyOpt. You might want to look at what other types of metadata should be preserved during these optimizations. Anything not given in the known ids list will be dropped. |
llvm/lib/Transforms/Utils/Local.cpp
Outdated
| @@ -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. | |||
There was a problem hiding this comment.
Shouldn't this be merging the contents of the memprof and callsite metadata, rather than taking it from one of the instructions?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
combineMetadata is typically used via combineMetadataForCSE, and both should be updated together. The usage in MemCpyOptimizer is quite unusual, as what we do there is not real CSE or instruction merging, so combineMetadata is not the best fit. That case needs to intersect AA attributes, but could keep all the non-AA/memory attributes of the original call. We don't have a great API to represent that use-case. The only other usage in InstCombine looks like it should just be using combineMetadataForCSE instead of maintaining its own metadata list. |
I agree - @DianQK can you change MemCpyOptimizer to intersect the AA attributes without calling combineMetadata? Once that is done it should be straightforward to change the call from InstCombine to use combineMetadataForCSE, at which point combineMetadata should likely be just inlined into combineMetadataForCSE to prevent its use from expanding. |
(In the meantime though I would like to get this PR in so we stop dropping this metadata on the floor) |
Yeah, I will do. |
Will do on the issue and FIXME |
metadata merge functions for memprof and callsite metadata.
| if (!(A && B)) { | ||
| return A ? A : B; | ||
| } | ||
|
|
||
| // TODO: Support more sophisticated merging, such as selecting the one with | ||
| // more bytes allocated, or implement support for carrying multiple allocation | ||
| // leaf contexts. For now, keep the first one. | ||
| return A; |
There was a problem hiding this comment.
This logic is a little hard to read. Can this be replaced with --
if(A) return A;
return B;
…on (#121359) This patch fixes a couple of places where memprof-related metadata (!memprof and !callsite) were being dropped, and one place where PGO metadata (!prof) was being dropped. All were due to instances of combineMetadata() being invoked. That function drops all metadata not in the list provided by the client, and also drops any not in its switch statement. Memprof metadata needed a case in the combineMetadata switch statement. For now we simply keep the metadata of the instruction being kept, which doesn't retain all the profile information when two calls with memprof metadata are being combined, but at least retains some. For the memprof metadata being dropped during call CSE, add memprof and callsite metadata to the list of known ids in combineMetadataForCSE. Neither memprof nor regular prof metadata were in the list of known ids for the callsite in MemCpyOptimizer, which was added to combine AA metadata after optimization of byval arguments fed by memcpy instructions, and similar types of optimizations of memcpy uses. There is one other callsite of combineMetadata, but it is only invoked on load instructions, which do not carry these types of metadata.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/11061 Here is the relevant piece of the build log for the reference |
This patch fixes a couple of places where memprof-related metadata
(!memprof and !callsite) were being dropped, and one place where PGO
metadata (!prof) was being dropped.
All were due to instances of combineMetadata() being invoked. That
function drops all metadata not in the list provided by the client, and
also drops any not in its switch statement.
Memprof metadata needed a case in the combineMetadata switch statement.
For now we simply keep the metadata of the instruction being kept, which
doesn't retain all the profile information when two calls with
memprof metadata are being combined, but at least retains some.
For the memprof metadata being dropped during call CSE, add memprof and
callsite metadata to the list of known ids in combineMetadataForCSE.
Neither memprof nor regular prof metadata were in the list of known ids
for the callsite in MemCpyOptimizer, which was added to combine AA
metadata after optimization of byval arguments fed by memcpy
instructions, and similar types of optimizations of memcpy uses.
There is one other callsite of combineMetadata, but it is only invoked
on load instructions, which do not carry these types of metadata.