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] Legalize <1 x T> vector types for atomic load #120385

Open
wants to merge 1 commit into
base: users/jofrn/spr/main/72529270
Choose a base branch
from

Conversation

jofrn
Copy link
Contributor

@jofrn jofrn commented Dec 18, 2024

@llvmbot
Copy link
Member

llvmbot commented Dec 18, 2024

@llvm/pr-subscribers-llvm-selectiondag

@llvm/pr-subscribers-backend-x86

Author: None (jofrn)

Changes

Scalarize vector of atomic load in SelectionDAG.


Stack:

  • #120387
  • #120386
  • #120385 ⬅
  • #120384

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.


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

3 Files Affected:

  • (modified) llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h (+1)
  • (modified) llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp (+15)
  • (modified) llvm/test/CodeGen/X86/atomic-load-store.ll (+9)
diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h b/llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
index 571a710cc92a34..b81c9f87cb27d7 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
+++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
@@ -861,6 +861,7 @@ class LLVM_LIBRARY_VISIBILITY DAGTypeLegalizer {
   SDValue ScalarizeVecRes_ExpOp(SDNode *N);
   SDValue ScalarizeVecRes_INSERT_VECTOR_ELT(SDNode *N);
   SDValue ScalarizeVecRes_LOAD(LoadSDNode *N);
+  SDValue ScalarizeVecRes_ATOMIC_LOAD(AtomicSDNode *N);
   SDValue ScalarizeVecRes_SCALAR_TO_VECTOR(SDNode *N);
   SDValue ScalarizeVecRes_VSELECT(SDNode *N);
   SDValue ScalarizeVecRes_SELECT(SDNode *N);
diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp b/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
index 107454a92e356c..c85e4ba2cfa5a7 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
@@ -60,6 +60,9 @@ void DAGTypeLegalizer::ScalarizeVectorResult(SDNode *N, unsigned ResNo) {
   case ISD::FP_ROUND:          R = ScalarizeVecRes_FP_ROUND(N); break;
   case ISD::FPOWI:             R = ScalarizeVecRes_ExpOp(N); break;
   case ISD::INSERT_VECTOR_ELT: R = ScalarizeVecRes_INSERT_VECTOR_ELT(N); break;
+  case ISD::ATOMIC_LOAD:
+    R = ScalarizeVecRes_ATOMIC_LOAD(cast<AtomicSDNode>(N));
+    break;
   case ISD::LOAD:           R = ScalarizeVecRes_LOAD(cast<LoadSDNode>(N));break;
   case ISD::SCALAR_TO_VECTOR:  R = ScalarizeVecRes_SCALAR_TO_VECTOR(N); break;
   case ISD::SIGN_EXTEND_INREG: R = ScalarizeVecRes_InregOp(N); break;
@@ -451,6 +454,18 @@ SDValue DAGTypeLegalizer::ScalarizeVecRes_INSERT_VECTOR_ELT(SDNode *N) {
   return Op;
 }
 
+SDValue DAGTypeLegalizer::ScalarizeVecRes_ATOMIC_LOAD(AtomicSDNode *N) {
+  SDValue Result = DAG.getAtomic(
+      ISD::ATOMIC_LOAD, SDLoc(N), N->getMemoryVT().getVectorElementType(),
+      N->getValueType(0).getVectorElementType(), N->getChain(), N->getBasePtr(),
+      N->getMemOperand());
+
+  // Legalize the chain result - switch anything that used the old chain to
+  // use the new one.
+  ReplaceValueWith(SDValue(N, 1), Result.getValue(1));
+  return Result;
+}
+
 SDValue DAGTypeLegalizer::ScalarizeVecRes_LOAD(LoadSDNode *N) {
   assert(N->isUnindexed() && "Indexed vector load?");
 
diff --git a/llvm/test/CodeGen/X86/atomic-load-store.ll b/llvm/test/CodeGen/X86/atomic-load-store.ll
index 5bce4401f7bdb0..9cac8167542d8b 100644
--- a/llvm/test/CodeGen/X86/atomic-load-store.ll
+++ b/llvm/test/CodeGen/X86/atomic-load-store.ll
@@ -28,3 +28,12 @@ define i32 @test3(ptr %ptr) {
   %val = load atomic i32, ptr %ptr seq_cst, align 4
   ret i32 %val
 }
+
+define <1 x i32> @atomic_vec1_i32(ptr %x) {
+; CHECK-LABEL: atomic_vec1_i32:
+; CHECK:       ## %bb.0:
+; CHECK-NEXT:    movl (%rdi), %eax
+; CHECK-NEXT:    retq
+  %ret = load atomic <1 x i32>, ptr %x acquire, align 4
+  ret <1 x i32> %ret
+}

@jofrn jofrn force-pushed the users/jofrn/spr/main/5c36cc8c branch from 452731d to e294f9f Compare December 18, 2024 08:54
@jofrn jofrn force-pushed the users/jofrn/spr/main/72529270 branch from b28c988 to b649f99 Compare December 18, 2024 11:45
@jofrn jofrn force-pushed the users/jofrn/spr/main/5c36cc8c branch from e294f9f to 7263545 Compare December 18, 2024 11:45
@RKSimon RKSimon requested review from arsenm and RKSimon December 18, 2024 13:56
@jofrn jofrn force-pushed the users/jofrn/spr/main/72529270 branch from b649f99 to 7d979a9 Compare December 18, 2024 19:11
@jofrn jofrn force-pushed the users/jofrn/spr/main/5c36cc8c branch 2 times, most recently from 5a3a12d to 66eca4b Compare December 18, 2024 20:47
@jyknight
Copy link
Member

Running llc -mtriple=x86_64-linux-gnu on

define <2 x i32> @atomic_vec2_i32(ptr %x) #0 {
  %ret = load atomic <2 x i32>, ptr %x acquire, align 64
  ret <2 x i32> %ret
}

crashes with:

WidenVectorResult #0: t3: v2i32,ch = AtomicLoad<(load acquire (s64) from %ir.x, align 64)> t0, t2
LLVM ERROR: Do not know how to widen the result of this operator!

@jyknight jyknight self-requested a review December 18, 2024 23:12
@arsenm
Copy link
Contributor

arsenm commented Dec 19, 2024

crashes with:

WidenVectorResult #0: t3: v2i32,ch = AtomicLoad<(load acquire (s64) from %ir.x, align 64)> t0, t2
LLVM ERROR: Do not know how to widen the result of this operator!

At this PR, this is the expectation. A later PR needs to handle the other vector legalization cases

@jofrn jofrn force-pushed the users/jofrn/spr/main/5c36cc8c branch from 66eca4b to 4d3fcb3 Compare December 19, 2024 02:29
@jofrn jofrn force-pushed the users/jofrn/spr/main/72529270 branch from bd31cd4 to 4282e9f Compare December 19, 2024 02:29
; CHECK0-NEXT: ## implicit-def: $xmm0
; CHECK0-NEXT: pinsrw $0, %eax, %xmm0
; CHECK0-NEXT: retq
%ret = load atomic <1 x bfloat>, ptr %x acquire, align 4
Copy link
Contributor

Choose a reason for hiding this comment

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

Overaligned

; CHECK0-NEXT: movw (%rdi), %ax
; CHECK0-NEXT: movswq %ax, %rax
; CHECK0-NEXT: retq
%ret = load atomic <1 x i16>, ptr %x acquire, align 4
Copy link
Contributor

Choose a reason for hiding this comment

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

Over aligned (but doesn't really matter)

@jofrn jofrn force-pushed the users/jofrn/spr/main/5c36cc8c branch from 4d3fcb3 to 4d0be71 Compare December 19, 2024 13:16
@jyknight
Copy link
Member

At this PR, this is the expectation. A later PR needs to handle the other vector legalization cases

Well, it also doesn't work after the rest of the series.

As a general comment, I find it difficult to review a series of PRs where the intermediate state between them is broken, and without any clear indication as to what is and isn't supposed to work after each PR.

If this set of changes is going to remain split up into multiple PRs, the descriptions need to make it clear what's the expected state after each one, so I don't need to guess as to which of the PRs to leave a comment like that on...

@jofrn jofrn force-pushed the users/jofrn/spr/main/5c36cc8c branch from 4d0be71 to 601c009 Compare December 19, 2024 16:01
@jofrn jofrn force-pushed the users/jofrn/spr/main/72529270 branch from 4d8ce80 to e32bf02 Compare December 19, 2024 16:01
@jofrn
Copy link
Contributor Author

jofrn commented Dec 19, 2024

As a general comment, I find it difficult to review a series of PRs where the intermediate state between them is broken, and without any clear indication as to what is and isn't supposed to work after each PR.

The tests posted in the review work from that point on, except for when the atomics are lowered to calls in #120387 ; these have always worked. I'm posting another review to handle more cases.

@jofrn jofrn force-pushed the users/jofrn/spr/main/5c36cc8c branch from 72ab88b to 35b6ddc Compare January 7, 2025 15:31
@jofrn jofrn force-pushed the users/jofrn/spr/main/5c36cc8c branch from 35b6ddc to 11174c5 Compare January 15, 2025 11:52
@jofrn jofrn force-pushed the users/jofrn/spr/main/72529270 branch from 7fa5c04 to 0dc865a Compare January 15, 2025 11:52
@jofrn jofrn force-pushed the users/jofrn/spr/main/5c36cc8c branch 3 times, most recently from 11b950d to ff979b4 Compare January 21, 2025 17:13
@jofrn jofrn force-pushed the users/jofrn/spr/main/72529270 branch from 19358e8 to 5fd5631 Compare January 21, 2025 17:13
@jofrn jofrn force-pushed the users/jofrn/spr/main/5c36cc8c branch from ff979b4 to e3d8ddd Compare January 21, 2025 17:44
@jofrn jofrn force-pushed the users/jofrn/spr/main/72529270 branch from 5fd5631 to 6444a74 Compare January 21, 2025 17:44
@jofrn jofrn force-pushed the users/jofrn/spr/main/5c36cc8c branch from e3d8ddd to 02d1d2d Compare January 21, 2025 17:50
@jofrn jofrn force-pushed the users/jofrn/spr/main/72529270 branch from 6444a74 to 28d94e9 Compare January 21, 2025 17:50
@jofrn jofrn force-pushed the users/jofrn/spr/main/72529270 branch from 28d94e9 to e3f4940 Compare January 22, 2025 11:05
@jofrn jofrn force-pushed the users/jofrn/spr/main/5c36cc8c branch 2 times, most recently from 8d438cf to c7f5fea Compare January 22, 2025 11:43
@jofrn jofrn force-pushed the users/jofrn/spr/main/72529270 branch from e3f4940 to 87b7090 Compare January 22, 2025 11:43
@jofrn jofrn force-pushed the users/jofrn/spr/main/5c36cc8c branch from c7f5fea to 9be2b4d Compare January 22, 2025 17:38
@jofrn jofrn force-pushed the users/jofrn/spr/main/72529270 branch 2 times, most recently from 0211266 to d4f4fa8 Compare January 22, 2025 17:47
@jofrn jofrn force-pushed the users/jofrn/spr/main/5c36cc8c branch 2 times, most recently from a1143a0 to 2a1b149 Compare January 22, 2025 18:19
@jofrn jofrn force-pushed the users/jofrn/spr/main/72529270 branch from d4f4fa8 to 1ee9bde Compare January 22, 2025 18:19
@jofrn jofrn changed the base branch from users/jofrn/spr/main/72529270 to main February 2, 2025 20:25
@jofrn jofrn force-pushed the users/jofrn/spr/main/5c36cc8c branch from 2a1b149 to d536a40 Compare February 2, 2025 20:25
@jofrn jofrn changed the base branch from main to users/jofrn/spr/main/72529270 February 2, 2025 20:25
`load atomic <1 x T>` is not valid. This change legalizes
vector types of atomic load via scalarization in SelectionDAG
so that it can, for example, translate from `v1i32` to `i32`.

commit-id:5c36cc8c
@jofrn jofrn force-pushed the users/jofrn/spr/main/72529270 branch from ff2e168 to 6a5ac3f Compare March 3, 2025 23:26
@jofrn jofrn force-pushed the users/jofrn/spr/main/5c36cc8c branch from d536a40 to 24d9628 Compare March 3, 2025 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:X86 llvm:SelectionDAG SelectionDAGISel as well
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants