Skip to content

Conversation

@nhaehnle
Copy link
Collaborator

@nhaehnle nhaehnle commented Nov 3, 2025

There can only be meaningful aliasing between the memory accesses of
different instructions if at least one of the accesses modifies memory.

This check is applied at the instruction-level earlier in the method.
This change merely extends the check on a per-MMO basis.

This affects a SystemZ test because PFD instructions are both mayLoad
and mayStore but may carry a load-only MMO which is now no longer
treated as aliasing loads. The PFD instructions are from llvm.prefetch
generated by loop-data-prefetch.


Stack:

⚠️ Part of a stack created by spr. Merging this PR using the GitHub UI may have unexpected results.

@llvmbot
Copy link
Member

llvmbot commented Nov 3, 2025

@llvm/pr-subscribers-backend-systemz

Author: Nicolai Hähnle (nhaehnle)

Changes

There can only be meaningful aliasing between the memory accesses of
different instructions if at least one of the accesses modifies memory.

This check is applied at the instruction-level earlier in the method.
This change merely extends the check on a per-MMO basis.

This affects a SystemZ test because PFD instructions are both mayLoad
and mayStore but may carry a load-only MMO which is now no longer
treated as aliasing loads. The PFD instructions are from llvm.prefetch
generated by loop-data-prefetch.


Stack:

  • [5/5] #166213
  • [4/5] #166212
  • [3/5] #166211 ⬅
  • [2/5] #166210
  • [1/5] #166209

⚠️ Part of a stack created by spr. Merging this PR using the GitHub UI may have unexpected results.


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

2 Files Affected:

  • (modified) llvm/lib/CodeGen/MachineInstr.cpp (+6-2)
  • (modified) llvm/test/CodeGen/SystemZ/vec-load-element.ll (+2-2)
diff --git a/llvm/lib/CodeGen/MachineInstr.cpp b/llvm/lib/CodeGen/MachineInstr.cpp
index 8ad9245a47684..37e5c517d24d8 100644
--- a/llvm/lib/CodeGen/MachineInstr.cpp
+++ b/llvm/lib/CodeGen/MachineInstr.cpp
@@ -1547,10 +1547,14 @@ bool MachineInstr::mayAlias(BatchAAResults *AA, const MachineInstr &Other,
 
   // Check each pair of memory operands from both instructions, which can't
   // alias only if all pairs won't alias.
-  for (auto *MMOa : memoperands())
-    for (auto *MMOb : Other.memoperands())
+  for (auto *MMOa : memoperands()) {
+    for (auto *MMOb : Other.memoperands()) {
+      if (!MMOa->isStore() && !MMOb->isStore())
+        continue;
       if (MemOperandsHaveAlias(MFI, AA, UseTBAA, MMOa, MMOb))
         return true;
+    }
+  }
 
   return false;
 }
diff --git a/llvm/test/CodeGen/SystemZ/vec-load-element.ll b/llvm/test/CodeGen/SystemZ/vec-load-element.ll
index 2baaed19546df..9bef279d7c0fa 100644
--- a/llvm/test/CodeGen/SystemZ/vec-load-element.ll
+++ b/llvm/test/CodeGen/SystemZ/vec-load-element.ll
@@ -5,8 +5,8 @@
 ; CHECK-LABEL: .LBB0_1:
 ; CHECK-NOT: l %r
 ; CHECK-NOT: vlvgf
-; CHECK: pfd
-; CHECK: vlef
+; CHECK-DAG: pfd
+; CHECK-DAG: vlef
 
 %type0 = type { i32, [400 x i8], i32, i32, i32, i32, i32, i32, i32, i32, i32, i32 }
 @Mem = external global [150 x %type0], align 4

Copy link
Contributor

@jayfoad jayfoad left a comment

Choose a reason for hiding this comment

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

This patch highlights a problem that I have run into before: if we want to support a single opcode (in this case BUNDLE) that may or may not load or store depending on its operands, then the current definitions of MachineInstr::mayLoad and mayStore are not good enough, because they just check a static property of the opcode.

If mayLoad and mayStore were more sophisticated then this check would have been handled already at L1527.

Having said that, in the short term I have no objection to this patch.

@nhaehnle nhaehnle changed the base branch from users/nhaehnle/spr/main/6596fbee to main November 4, 2025 18:59
@nhaehnle nhaehnle force-pushed the users/nhaehnle/spr/main/667859fc branch from e64b325 to c3abece Compare November 4, 2025 18:59
@nhaehnle nhaehnle changed the base branch from main to users/nhaehnle/spr/main/6596fbee November 4, 2025 18:59
@nhaehnle
Copy link
Collaborator Author

nhaehnle commented Nov 5, 2025

This patch highlights a problem that I have run into before: if we want to support a single opcode (in this case BUNDLE) that may or may not load or store depending on its operands, then the current definitions of MachineInstr::mayLoad and mayStore are not good enough, because they just check a static property of the opcode.

A potential solution to this problem would be an empty MMO, i.e. an MMO that can never alias anything.

If mayLoad and mayStore were more sophisticated then this check would have been handled already at L1527.

This change aims at something subtly different. What it aims to do is improve the alias checking of two instructions with MMOs:

  • load X, store Y
  • load X

These two instructions don't alias, since their only overlap is in the load MMOs, which isn't a conflict.

@nhaehnle nhaehnle force-pushed the users/nhaehnle/spr/main/667859fc branch from c3abece to 0470587 Compare November 5, 2025 01:43
@nhaehnle nhaehnle force-pushed the users/nhaehnle/spr/main/6596fbee branch from 764dc17 to fe082d1 Compare November 5, 2025 01:43
@nhaehnle nhaehnle force-pushed the users/nhaehnle/spr/main/667859fc branch from 0470587 to f40a20f Compare November 5, 2025 06:15
@nhaehnle nhaehnle force-pushed the users/nhaehnle/spr/main/6596fbee branch from fe082d1 to e59b115 Compare November 5, 2025 06:15
Base automatically changed from users/nhaehnle/spr/main/6596fbee to main November 5, 2025 06:56
@jayfoad
Copy link
Contributor

jayfoad commented Nov 5, 2025

If mayLoad and mayStore were more sophisticated then this check would have been handled already at L1527.

This change aims at something subtly different. What it aims to do is improve the alias checking of two instructions with MMOs:

  • load X, store Y
  • load X

These two instructions don't alias, since their only overlap is in the load MMOs, which isn't a conflict.

Yeah, I realized that later. LGTM.

There can only be meaningful aliasing between the memory accesses of
different instructions if at least one of the accesses modifies memory.

This check is applied at the instruction-level earlier in the method.
This change merely extends the check on a per-MMO basis.

This affects a SystemZ test because PFD instructions are both mayLoad
and mayStore but may carry a load-only MMO which is now no longer
treated as aliasing loads. The PFD instructions are from llvm.prefetch
generated by loop-data-prefetch.

commit-id:667859fc
@nhaehnle nhaehnle force-pushed the users/nhaehnle/spr/main/667859fc branch from f40a20f to 4e20232 Compare November 6, 2025 16:12
@nhaehnle nhaehnle enabled auto-merge (squash) November 6, 2025 16:15
@nhaehnle
Copy link
Collaborator Author

nhaehnle commented Nov 6, 2025

The Windows CI reports a failure, but the logs look completely clean except for this error that seems to originate from the CI infra itself:

2025-11-06T16:56:57.0555141Z + python C:/_work/llvm-project/llvm-project/.ci/generate_test_report_github.py 0 C:/_work/llvm-project/llvm-project/build/test-results.7hrcixpi.xml C:/_work/llvm-project/llvm-project/build/test-results._337lz6b.xml C:/_work/llvm-project/llvm-project/build/test-results.bkrbjuib.xml C:/_work/llvm-project/llvm-project/build/test-results.g3dbdeym.xml C:/_work/llvm-project/llvm-project/build/test-results.ll76ye1t.xml C:/_work/llvm-project/llvm-project/build/test-results.y4zoj7fv.xml C:/_work/llvm-project/llvm-project/ninja.log
2025-11-06T16:56:57.1272333Z Traceback (most recent call last):
2025-11-06T16:56:57.1272859Z   File "C:\_work\llvm-project\llvm-project\.ci\generate_test_report_github.py", line 9, in <module>
2025-11-06T16:56:57.1274006Z     import generate_test_report_lib
2025-11-06T16:56:57.1274366Z   File "C:\_work\llvm-project\llvm-project\.ci\generate_test_report_lib.py", line 13, in <module>
2025-11-06T16:56:57.1275439Z     class FailureExplanation(TypedDict):
2025-11-06T16:56:57.1275849Z   File "C:\_work\llvm-project\llvm-project\.ci\generate_test_report_lib.py", line 16, in FailureExplanation
2025-11-06T16:56:57.1276567Z     reason: str | None
2025-11-06T16:56:57.1276844Z TypeError: unsupported operand type(s) for |: 'type' and 'NoneType'
2025-11-06T16:56:57.1404087Z ##[error]Process completed with exit code 1.

@nhaehnle nhaehnle disabled auto-merge November 6, 2025 17:18
@nhaehnle nhaehnle merged commit d1387ed into main Nov 6, 2025
12 of 17 checks passed
@nhaehnle nhaehnle deleted the users/nhaehnle/spr/main/667859fc branch November 6, 2025 17:19
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.

4 participants