Skip to content

[AggressiveInstCombine] Use AA during store merge #149992

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.

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 1 commit into from
Jul 23, 2025

Conversation

nikic
Copy link
Contributor

@nikic nikic commented Jul 22, 2025

This is a small extension of #147540, resolving one of the FIXMEs. Instead of bailing out on any instruction that may read/write memory, use AA to check whether it can alias the stored parts. Do this using a crude check based on the underlying object only.

This pattern occurs rarely in practice (dtcxzyw/llvm-opt-benchmark#2597 shows it only in llvm), but at the same time it also doesn't seem to add any compile-time cost (https://llvm-compile-time-tracker.com/compare.php?from=cb8b0cd2cfbe817253f2679df53dd7926a7e1894&to=c4ab835e1d9b040ba5882238fe3e56ff32d314a8&stat=instructions:u), so it's probably worth handling.

This is a small extension of llvm#147540, resolving one of the FIXME.
Instead of bailing out on any instruction that may read/write
memory, use AA to check whether it can alias the stored parts.
Do this using a crude check based on the underlying object only.
@nikic nikic requested review from fhahn and dtcxzyw July 22, 2025 10:23
@llvmbot
Copy link
Member

llvmbot commented Jul 22, 2025

@llvm/pr-subscribers-llvm-transforms

Author: Nikita Popov (nikic)

Changes

This is a small extension of #147540, resolving one of the FIXMEs. Instead of bailing out on any instruction that may read/write memory, use AA to check whether it can alias the stored parts. Do this using a crude check based on the underlying object only.

This pattern occurs rarely in practice (dtcxzyw/llvm-opt-benchmark#2597 shows it only in llvm), but at the same time it also doesn't seem to add any compile-time cost (https://llvm-compile-time-tracker.com/compare.php?from=cb8b0cd2cfbe817253f2679df53dd7926a7e1894&to=c4ab835e1d9b040ba5882238fe3e56ff32d314a8&stat=instructions:u), so it's probably worth handling.


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp (+8-2)
  • (modified) llvm/test/Transforms/AggressiveInstCombine/X86/store-merge.ll (+2-12)
diff --git a/llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp b/llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp
index 7fa6e6c5161cf..7af5ba4e0e103 100644
--- a/llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp
+++ b/llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp
@@ -965,6 +965,7 @@ static bool foldConsecutiveStores(BasicBlock &BB, const DataLayout &DL,
   if (DL.isBigEndian())
     return false;
 
+  BatchAAResults BatchAA(AA);
   SmallVector<PartStore, 8> Parts;
   bool MadeChange = false;
   for (Instruction &I : make_early_inc_range(BB)) {
@@ -980,8 +981,13 @@ static bool foldConsecutiveStores(BasicBlock &BB, const DataLayout &DL,
       continue;
     }
 
-    // FIXME: Use AA to make this more precise.
-    if (I.mayReadOrWriteMemory() || I.mayThrow()) {
+    if (Parts.empty())
+      continue;
+
+    if (I.mayThrow() ||
+        (I.mayReadOrWriteMemory() &&
+         isModOrRefSet(BatchAA.getModRefInfo(
+             &I, MemoryLocation::getBeforeOrAfter(Parts[0].PtrBase))))) {
       MadeChange |= mergePartStores(Parts, DL, TTI);
       Parts.clear();
       continue;
diff --git a/llvm/test/Transforms/AggressiveInstCombine/X86/store-merge.ll b/llvm/test/Transforms/AggressiveInstCombine/X86/store-merge.ll
index 4ab8d18eb69b5..56786d0f9def0 100644
--- a/llvm/test/Transforms/AggressiveInstCombine/X86/store-merge.ll
+++ b/llvm/test/Transforms/AggressiveInstCombine/X86/store-merge.ll
@@ -359,13 +359,8 @@ define void @test_aliasing_store(i16 %x, ptr %p, ptr %p2) {
 define void @test_non_aliasing_store(i16 %x, ptr noalias %p, ptr noalias %p2) {
 ; CHECK-LABEL: define void @test_non_aliasing_store(
 ; CHECK-SAME: i16 [[X:%.*]], ptr noalias [[P:%.*]], ptr noalias [[P2:%.*]]) {
-; CHECK-NEXT:    [[X_0:%.*]] = trunc i16 [[X]] to i8
-; CHECK-NEXT:    store i8 [[X_0]], ptr [[P]], align 1
+; CHECK-NEXT:    store i16 [[X]], ptr [[P]], align 1
 ; CHECK-NEXT:    store i8 0, ptr [[P2]], align 1
-; CHECK-NEXT:    [[SHR_1:%.*]] = lshr i16 [[X]], 8
-; CHECK-NEXT:    [[X_1:%.*]] = trunc i16 [[SHR_1]] to i8
-; CHECK-NEXT:    [[GEP_1:%.*]] = getelementptr i8, ptr [[P]], i64 1
-; CHECK-NEXT:    store i8 [[X_1]], ptr [[GEP_1]], align 1
 ; CHECK-NEXT:    ret void
 ;
   %x.0 = trunc i16 %x to i8
@@ -403,13 +398,8 @@ define i8 @test_aliasing_load(i16 %x, ptr %p, ptr %p2) {
 define i8 @test_non_aliasing_load(i16 %x, ptr noalias %p, ptr noalias %p2) {
 ; CHECK-LABEL: define i8 @test_non_aliasing_load(
 ; CHECK-SAME: i16 [[X:%.*]], ptr noalias [[P:%.*]], ptr noalias [[P2:%.*]]) {
-; CHECK-NEXT:    [[X_0:%.*]] = trunc i16 [[X]] to i8
-; CHECK-NEXT:    store i8 [[X_0]], ptr [[P]], align 1
+; CHECK-NEXT:    store i16 [[X]], ptr [[P]], align 1
 ; CHECK-NEXT:    [[V:%.*]] = load i8, ptr [[P2]], align 1
-; CHECK-NEXT:    [[SHR_1:%.*]] = lshr i16 [[X]], 8
-; CHECK-NEXT:    [[X_1:%.*]] = trunc i16 [[SHR_1]] to i8
-; CHECK-NEXT:    [[GEP_1:%.*]] = getelementptr i8, ptr [[P]], i64 1
-; CHECK-NEXT:    store i8 [[X_1]], ptr [[GEP_1]], align 1
 ; CHECK-NEXT:    ret i8 [[V]]
 ;
   %x.0 = trunc i16 %x to i8

@nikic nikic merged commit b17f4d3 into llvm:main Jul 23, 2025
10 of 11 checks passed
@nikic nikic deleted the store-merge-alias branch July 23, 2025 08:10
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