Skip to content
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

[SelectionDAG] Remove pointer from MMO for VP strided load/store. #82667

Merged
merged 3 commits into from
Feb 27, 2024

Conversation

topperc
Copy link
Collaborator

@topperc topperc commented Feb 22, 2024

MachineIR alias analysis assumes that only bytes after the pointer will be accessed. This is incorrect if the stride is negative.

This is causing miscompiles in our downstream after SLP started making strided loads.

Fixes #82657

MachineIR alias analysis assumes that only bytes after the pointer
will be accessed. This is incorrect if the stride is negative.
@topperc topperc requested a review from preames February 22, 2024 18:15
@llvmbot llvmbot added the llvm:SelectionDAG SelectionDAGISel as well label Feb 22, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 22, 2024

@llvm/pr-subscribers-llvm-selectiondag

Author: Craig Topper (topperc)

Changes

MachineIR alias analysis assumes that only bytes after the pointer will be accessed. This is incorrect if the stride is negative.

This is causing miscompiles in our downstream after SLP started making strided loads.


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

1 Files Affected:

  • (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp (+4-2)
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index e893a5b616d33e..3e53f3318a374a 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -8017,8 +8017,9 @@ void SelectionDAGBuilder::visitVPStridedLoad(
   MemoryLocation ML = MemoryLocation::getAfter(PtrOperand, AAInfo);
   bool AddToChain = !AA || !AA->pointsToConstantMemory(ML);
   SDValue InChain = AddToChain ? DAG.getRoot() : DAG.getEntryNode();
+  unsigned AS = PtrOperand->getPointerAddressSpace();
   MachineMemOperand *MMO = DAG.getMachineFunction().getMachineMemOperand(
-      MachinePointerInfo(PtrOperand), MachineMemOperand::MOLoad,
+      MachinePointerInfo(AS), MachineMemOperand::MOLoad,
       MemoryLocation::UnknownSize, *Alignment, AAInfo, Ranges);
 
   SDValue LD = DAG.getStridedLoadVP(VT, DL, InChain, OpValues[0], OpValues[1],
@@ -8039,8 +8040,9 @@ void SelectionDAGBuilder::visitVPStridedStore(
   if (!Alignment)
     Alignment = DAG.getEVTAlign(VT.getScalarType());
   AAMDNodes AAInfo = VPIntrin.getAAMetadata();
+  unsigned AS = PtrOperand->getPointerAddressSpace();
   MachineMemOperand *MMO = DAG.getMachineFunction().getMachineMemOperand(
-      MachinePointerInfo(PtrOperand), MachineMemOperand::MOStore,
+      MachinePointerInfo(AS), MachineMemOperand::MOStore,
       MemoryLocation::UnknownSize, *Alignment, AAInfo);
 
   SDValue ST = DAG.getStridedStoreVP(

Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

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

testcase?

@topperc
Copy link
Collaborator Author

topperc commented Feb 25, 2024

@arsenm do you want a test case for the scheduling issue? Or just a test to show we don't put a pointer in the MMO?

@arsenm
Copy link
Contributor

arsenm commented Feb 26, 2024

@arsenm do you want a test case for the scheduling issue? Or just a test to show we don't put a pointer in the MMO?

Either I guess. The MMO is more targeted but the scheduling is more interesting

@topperc topperc merged commit 62d0c01 into llvm:main Feb 27, 2024
3 of 4 checks passed
@topperc topperc deleted the pr/strided-mmo branch February 27, 2024 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm:SelectionDAG SelectionDAGISel as well
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MachineScheduler Alias analysis broken for vp.strided.load/store with negative stride
3 participants