Skip to content

[MemCpyOpt] Merge alias metadatas when replacing arguments#67539

Merged
nikic merged 1 commit into
llvm:mainfrom
dianqk:memcpyopt-alias
Sep 28, 2023
Merged

[MemCpyOpt] Merge alias metadatas when replacing arguments#67539
nikic merged 1 commit into
llvm:mainfrom
dianqk:memcpyopt-alias

Conversation

@dianqk

@dianqk dianqk commented Sep 27, 2023

Copy link
Copy Markdown
Member

Fix mis-compilation encountered in https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Failing.20tests.20when.20rustc.20is.20compiled.20with.201.20CGU.

I marked it as a draft PR because I haven't got an appropriate test case yet.
We may want to merge pr without additional test cases first.

@dianqk dianqk requested review from khei4 and nikic September 27, 2023 10:50
@dianqk dianqk marked this pull request as ready for review September 27, 2023 21:26
@llvmbot

llvmbot commented Sep 27, 2023

Copy link
Copy Markdown
Member

@llvm/pr-subscribers-llvm-transforms

Changes

Fix mis-compilation encountered in https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Failing.20tests.20when.20rustc.20is.20compiled.20with.201.20CGU.

I marked it as a draft PR because I haven't got an appropriate test case yet.


Full diff: https://github.com/llvm/llvm-project/pull/67539.diff

1 Files Affected:

  • (modified) llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp (+14-9)
diff --git a/llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp b/llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
index 4db9d1b6d309afd..e818f6d4e84627d 100644
--- a/llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
+++ b/llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
@@ -336,6 +336,17 @@ static bool writtenBetween(MemorySSA *MSSA, BatchAAResults &AA,
   return !MSSA->dominates(Clobber, Start);
 }
 
+// Update AA metadata
+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};
+  combineMetadata(ReplInst, I, KnownIDs, true);
+}
+
 /// When scanning forward over instructions, we look for some other patterns to
 /// fold away. In particular, this looks for stores to neighboring locations of
 /// memory. If it sees enough consecutive ones, it attempts to merge them
@@ -1096,16 +1107,9 @@ bool MemCpyOptPass::performCallSlotOptzn(Instruction *cpyLoad,
                       MSSA->getMemoryAccess(C));
   }
 
-  // Update AA metadata
-  // 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};
-  combineMetadata(C, cpyLoad, KnownIDs, true);
+  combineAAMetadata(C, cpyLoad);
   if (cpyLoad != cpyStore)
-    combineMetadata(C, cpyStore, KnownIDs, true);
+    combineAAMetadata(C, cpyStore);
 
   ++NumCallSlot;
   return true;
@@ -1961,6 +1965,7 @@ bool MemCpyOptPass::processImmutArgument(CallBase &CB, unsigned ArgNo) {
                     << "  " << CB << "\n");
 
   // Otherwise we're good!  Update the immut argument.
+  combineAAMetadata(&CB, MDep);
   CB.setArgOperand(ArgNo, MDep->getSource());
   ++NumMemCpyInstr;
   return true;

@nikic

nikic commented Sep 28, 2023

Copy link
Copy Markdown
Contributor

Here's a simple example for the memcpyopt+dse combination: https://llvm.godbolt.org/z/EnbPsqab1

nikic added a commit that referenced this pull request Sep 28, 2023

@nikic nikic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I've rebased this over a test case. LGTM

@nikic nikic merged commit 4e6e476 into llvm:main Sep 28, 2023
@dianqk

dianqk commented Sep 28, 2023

Copy link
Copy Markdown
Member Author

I've rebased this over a test case. LGTM

Thanks a lot!

@dianqk dianqk deleted the memcpyopt-alias branch September 28, 2023 09:26
nikic added a commit to rust-lang/llvm-project that referenced this pull request Sep 28, 2023
(cherry picked from commit d5c8b23)
nikic pushed a commit to rust-lang/llvm-project that referenced this pull request Sep 28, 2023
Alias metadata may no longer be valid after replacing the call argument.
Fix this by merging it with the memcpy alias metadata.

This fixes a miscompilation encountered in
https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Failing.20tests.20when.20rustc.20is.20compiled.20with.201.20CGU.

(cherry picked from commit 4e6e476)
tru pushed a commit that referenced this pull request Sep 29, 2023
(cherry picked from commit d5c8b23)
tru pushed a commit that referenced this pull request Sep 29, 2023
Alias metadata may no longer be valid after replacing the call argument.
Fix this by merging it with the memcpy alias metadata.

This fixes a miscompilation encountered in
https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Failing.20tests.20when.20rustc.20is.20compiled.20with.201.20CGU.

(cherry picked from commit 4e6e476)
legrosbuffle pushed a commit to legrosbuffle/llvm-project that referenced this pull request Sep 29, 2023
legrosbuffle pushed a commit to legrosbuffle/llvm-project that referenced this pull request Sep 29, 2023
Alias metadata may no longer be valid after replacing the call argument.
Fix this by merging it with the memcpy alias metadata.

This fixes a miscompilation encountered in
https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Failing.20tests.20when.20rustc.20is.20compiled.20with.201.20CGU.
Guzhu-AMD pushed a commit to GPUOpen-Drivers/llvm-project that referenced this pull request Oct 12, 2023
Local branch amd-gfx 5512cfe Merged main:34ee53c9e390 into amd-gfx:85bfd92dd479
Remote branch main 4e6e476 [MemCpyOpt] Merge alias metadatas when replacing arguments (llvm#67539)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants