[X86] Lowering of load atomic vector of float via cast #117189

jofrn · 2024-11-21T16:52:53Z

X86 backend does not lower load atomic <n x float>; yet it does so for integers, so we can cast to an integer before lowering in order to support this instruction.

llvmbot · 2024-11-21T16:53:28Z

@llvm/pr-subscribers-llvm-transforms
@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-backend-x86

Author: None (jofrn)

Changes

X86 backend does not lower load atomic float; yet it does so for integers, so we can cast to an integer before lowering in order to support this instruction.

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

3 Files Affected:

(modified) llvm/lib/Target/X86/X86ISelLowering.cpp (+8)
(modified) llvm/lib/Target/X86/X86ISelLowering.h (+2)
(added) llvm/test/CodeGen/X86/atomic-float.ll (+15)

diff --git a/llvm/lib/Target/X86/X86ISelLowering.cpp b/llvm/lib/Target/X86/X86ISelLowering.cpp
index bcb84add65d83e..b0abd1315bd812 100644
--- a/llvm/lib/Target/X86/X86ISelLowering.cpp
+++ b/llvm/lib/Target/X86/X86ISelLowering.cpp
@@ -30976,6 +30976,14 @@ bool X86TargetLowering::needsCmpXchgNb(Type *MemType) const {
   return false;
 }
 
+TargetLoweringBase::AtomicExpansionKind
+X86TargetLowering::shouldCastAtomicLoadInIR(LoadInst *LI) const {
+  if (LI->getType()->isVectorTy())
+    if (cast<VectorType>(LI->getType())->getElementType()->isFloatingPointTy())
+      return AtomicExpansionKind::CastToInteger;
+  return TargetLowering::shouldCastAtomicLoadInIR(LI);
+}
+
 TargetLoweringBase::AtomicExpansionKind
 X86TargetLowering::shouldExpandAtomicStoreInIR(StoreInst *SI) const {
   Type *MemType = SI->getValueOperand()->getType();
diff --git a/llvm/lib/Target/X86/X86ISelLowering.h b/llvm/lib/Target/X86/X86ISelLowering.h
index 14ada1721fd40e..75f8c46362327f 100644
--- a/llvm/lib/Target/X86/X86ISelLowering.h
+++ b/llvm/lib/Target/X86/X86ISelLowering.h
@@ -1808,6 +1808,8 @@ namespace llvm {
     const MCPhysReg *getScratchRegisters(CallingConv::ID CC) const override;
     ArrayRef<MCPhysReg> getRoundingControlRegisters() const override;
 
+    TargetLoweringBase::AtomicExpansionKind
+    shouldCastAtomicLoadInIR(LoadInst *LI) const override;
     TargetLoweringBase::AtomicExpansionKind
     shouldExpandAtomicLoadInIR(LoadInst *LI) const override;
     TargetLoweringBase::AtomicExpansionKind
diff --git a/llvm/test/CodeGen/X86/atomic-float.ll b/llvm/test/CodeGen/X86/atomic-float.ll
new file mode 100644
index 00000000000000..fa5977808aed51
--- /dev/null
+++ b/llvm/test/CodeGen/X86/atomic-float.ll
@@ -0,0 +1,15 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt < %s --mtriple=x86_64 --passes=atomic-expand -S -o - | FileCheck %s
+
+define float @load_atomic_float() {
+; CHECK-LABEL: define float @load_atomic_float() {
+; CHECK-NEXT:    [[SRC:%.*]] = alloca float, align 4
+; CHECK-NEXT:    [[TMP1:%.*]] = load atomic i32, ptr [[SRC]] acquire, align 4
+; CHECK-NEXT:    [[TMP2:%.*]] = bitcast i32 [[TMP1]] to float
+; CHECK-NEXT:    ret float [[TMP2]]
+;
+  %src = alloca float
+  %ret = load atomic float, ptr %src acquire, align 4
+  ret float %ret
+}
+

llvm/test/CodeGen/X86/atomic-float.ll

llvm/lib/Target/X86/X86ISelLowering.cpp

arsenm · 2024-11-22T00:18:12Z

llvm/lib/Target/X86/X86ISelLowering.cpp

@@ -30976,6 +30976,14 @@ bool X86TargetLowering::needsCmpXchgNb(Type *MemType) const {
  return false;
 }

+TargetLoweringBase::AtomicExpansionKind


I would like to get rid of this hook. Directly promoting atomic_load to int in codegen, like any other operation, shouldn't be difficult

phoebewang · 2024-11-22T03:24:39Z

llvm/lib/Target/X86/X86ISelLowering.cpp

@@ -30976,6 +30976,14 @@ bool X86TargetLowering::needsCmpXchgNb(Type *MemType) const {
  return false;
 }

+TargetLoweringBase::AtomicExpansionKind
+X86TargetLowering::shouldCastAtomicLoadInIR(LoadInst *LI) const {
+  if (LI->getType()->isVectorTy())


Add a test case for vector type?

Currently disallowed by the IR verifier (although we should fix this)

llvm/lib/IR/Verifier.cpp

shiltian · 2024-11-25T20:30:54Z

it seems like this PR is stacked but not properly stacked

github-actions · 2024-11-25T20:32:32Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Vector types on atomics are assumed to be invalid by the verifier. However, this type can be valid if it is lowered by codegen.

X86 backend does not lower load atomic float, so it can be casted to an integer before lowering.

llvm/test/CodeGen/X86/atomicvec-float.ll

arsenm · 2024-12-02T23:53:46Z

llvm/test/CodeGen/X86/atomicvec-float.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc < %s --mtriple=x86_64 | FileCheck %s
+
+define float @load_atomic_float(ptr %src) {


Test all the FP types, float, double, half, bfloat. fp128 if 128-bit atomics work

arsenm · 2024-12-02T23:54:28Z

llvm/lib/IR/Verifier.cpp

-    Check(ElTy->isIntOrPtrTy() || ElTy->isFloatingPointTy(),
-          "atomic load operand must have integer, pointer, or floating point "
-          "type!",
+    Check(ElTy->getScalarType()->isIntOrPtrTy() ||


This PR shouldn't touch the verifier, only handle codegen for atomic load / store

It is stacked on top of #117625. Please take a look. This PR handles vectors only -- renamed the title.

llvm/test/CodeGen/X86/atomicvec-float.ll

arsenm · 2024-12-02T23:57:01Z

llvm/lib/Target/X86/X86ISelLowering.cpp

+TargetLoweringBase::AtomicExpansionKind
+X86TargetLowering::shouldCastAtomicLoadInIR(LoadInst *LI) const {
+  if (const auto VT = dyn_cast<VectorType>(LI->getType()))
+    if (VT->getElementType()->isFloatingPointTy())
+      return AtomicExpansionKind::CastToInteger;
+  return TargetLowering::shouldCastAtomicLoadInIR(LI);
+}


This seems to be moving in the opposite direction. The existing tests atomic FP load already works, and this is regressing it to use the IR expansion (which should be deleted)

That one didn't handle atomic vectors. This PR only needs atomic load of vector type to pass. Casting it here is simple for now since the hook still exists.

llvm/lib/Target/X86/X86ISelLowering.cpp

arsenm · 2024-12-06T16:09:43Z

llvm/lib/Target/X86/X86ISelLowering.cpp

+  if (const auto VT = dyn_cast<VectorType>(LI->getType()))
+    if (VT->getElementType()->isFloatingPointTy())


Should move this into the default implementation. Targets can opt out as the hook is removed. Also this need not be limited to FP vectors. All vectors should cast to int

jofrn requested review from arsenm, shiltian and Pierre-vh November 21, 2024 16:52

llvmbot added the backend:X86 label Nov 21, 2024

arsenm reviewed Nov 22, 2024

View reviewed changes

phoebewang reviewed Nov 22, 2024

View reviewed changes

jofrn force-pushed the atomic-floatcast branch from 2d0ad97 to b95a021 Compare November 22, 2024 21:58

llvmbot added llvm:ir llvm:transforms labels Nov 22, 2024

arsenm reviewed Nov 22, 2024

View reviewed changes

llvm/lib/IR/Verifier.cpp Outdated Show resolved Hide resolved

jofrn force-pushed the atomic-floatcast branch from b95a021 to 8fd1c35 Compare November 25, 2024 20:29

jofrn force-pushed the atomic-floatcast branch from 8fd1c35 to f2b9675 Compare November 25, 2024 20:48

jofrn mentioned this pull request Nov 25, 2024

IR: Allow vector type in atomic load and store #117625

Closed

jofrn and others added 2 commits December 1, 2024 14:37

[Verifier] Allow vector type in atomic load and store

9b0a33b

Vector types on atomics are assumed to be invalid by the verifier. However, this type can be valid if it is lowered by codegen.

[X86] Lowering of load atomic float via cast

1eabd74

X86 backend does not lower load atomic float, so it can be casted to an integer before lowering.

jofrn force-pushed the atomic-floatcast branch from f2b9675 to 1eabd74 Compare December 1, 2024 19:46

arsenm reviewed Dec 2, 2024

View reviewed changes

llvm/lib/Target/X86/X86ISelLowering.cpp Show resolved Hide resolved

jofrn changed the title ~~[X86] Lowering of load atomic float via cast~~ [X86] Lowering of load atomic vector of float via cast Dec 4, 2024

Add tests for double,half,bfloat,fp128

027343e

arsenm reviewed Dec 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[X86] Lowering of load atomic vector of float via cast #117189

[X86] Lowering of load atomic vector of float via cast #117189

jofrn commented Nov 21, 2024 •

edited

Loading

llvmbot commented Nov 21, 2024 •

edited

Loading

arsenm Nov 22, 2024

phoebewang Nov 22, 2024

arsenm Nov 22, 2024

shiltian commented Nov 25, 2024

github-actions bot commented Nov 25, 2024 •

edited

Loading

arsenm Dec 2, 2024

arsenm Dec 2, 2024

jofrn Dec 4, 2024

arsenm Dec 2, 2024

jofrn Dec 4, 2024 •

edited

Loading

arsenm Dec 6, 2024 •

edited

Loading

		if (const auto VT = dyn_cast<VectorType>(LI->getType()))
		if (VT->getElementType()->isFloatingPointTy())

[X86] Lowering of load atomic vector of float via cast #117189

Are you sure you want to change the base?

[X86] Lowering of load atomic vector of float via cast #117189

Conversation

jofrn commented Nov 21, 2024 • edited Loading

llvmbot commented Nov 21, 2024 • edited Loading

arsenm Nov 22, 2024

Choose a reason for hiding this comment

phoebewang Nov 22, 2024

Choose a reason for hiding this comment

arsenm Nov 22, 2024

Choose a reason for hiding this comment

shiltian commented Nov 25, 2024

github-actions bot commented Nov 25, 2024 • edited Loading

arsenm Dec 2, 2024

Choose a reason for hiding this comment

arsenm Dec 2, 2024

Choose a reason for hiding this comment

jofrn Dec 4, 2024

Choose a reason for hiding this comment

arsenm Dec 2, 2024

Choose a reason for hiding this comment

jofrn Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

arsenm Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

jofrn commented Nov 21, 2024 •

edited

Loading

llvmbot commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 25, 2024 •

edited

Loading

jofrn Dec 4, 2024 •

edited

Loading

arsenm Dec 6, 2024 •

edited

Loading