Skip to content

[SPIRV] Handle aggregate arguments to spv_store #172348

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.

Sign up for GitHub

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

Open
AlexVlx wants to merge 10 commits into
base: main
Choose a base branch
from
Open

[SPIRV] Handle aggregate arguments to spv_store #172348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c0933bf
i128 is legal in IR / MIR, even though it might yield a module which …
AlexVlx Dec 9, 2025
82dbfe9
Fix formatting.
AlexVlx Dec 9, 2025
18c2124
Merge branch 'main' of https://github.com/llvm/llvm-project into spir…
AlexVlx Dec 11, 2025
2eb946a
Merge branch 'main' of https://github.com/llvm/llvm-project into spir…
AlexVlx Dec 11, 2025
40de8fd
Merge branch 'main' of https://github.com/llvm/llvm-project into spir…
AlexVlx Dec 12, 2025
97d5cc2
Handle storing of aggregates via `spv_store`.
AlexVlx Dec 15, 2025
46745f4
Merge branch 'main' of https://github.com/llvm/llvm-project into spir…
AlexVlx Dec 15, 2025
f837bb6
Fix formatting.
AlexVlx Dec 15, 2025
b72197d
Merge branch 'main' of https://github.com/llvm/llvm-project into spir…
AlexVlx Dec 15, 2025
f70786d
Merge branch 'main' of https://github.com/llvm/llvm-project into spir…
AlexVlx Dec 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2032,6 +2032,19 @@ Instruction *SPIRVEmitIntrinsics::visitStoreInst(StoreInst &I) {
MachineMemOperand::Flags Flags =
TLI->getStoreMemOperandFlags(I, CurrF->getDataLayout());
auto *PtrOp = I.getPointerOperand();

if (I.getValueOperand()->getType()->isAggregateType()) {
// It is possible that what used to be an ExtractValueInst has been replaced
// with a call to the spv_extractv intrinsic, and that said call hasn't
// had its return type replaced with i32 during the dedicated pass (because
// it was emitted later); we have to handle this here, because IRTranslator
// cannot deal with multi-register types at the moment.
CallBase *CB = dyn_cast<CallBase>(I.getValueOperand());
assert(CB && CB->getIntrinsicID() == Intrinsic::spv_extractv &&
"Unexpected argument of aggregate type, should be spv_extractv!");
CB->mutateType(B.getInt32Ty());
Comment on lines +2031 to +2034
Copy link
Contributor

@jmmartinez jmmartinez Dec 16, 2025

Choose a reason for hiding this comment

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

I'm not sure this change generates valid llvm-ir since we update the call return type but not the called function type.

The generated LLVM-IR for the test is:

define void @sneaky(ptr addrspace(4) %p, i32 %a) {
  %1 = call spir_func i32 @arr_ret()
  call void @llvm.spv.assign.type.i32(i32 %1, metadata %ty3 poison)
  call void @llvm.spv.assign.ptr.type.p4(ptr addrspace(4) %p, metadata %ty3 poison, i32 4)
  %2 = call ptr addrspace(4) (i1, ptr addrspace(4), ...) @llvm.spv.gep.p4.p4(i1 true, ptr addrspace(4) %p, i32 0, i32 2)
  call void @llvm.spv.assign.ptr.type.p4(ptr addrspace(4) %2, metadata [8 x i8] poison, i32 4)
  %3 = call [8 x i8] (i32, ...) @llvm.spv.extractv.a8i8(i32 %1, i32 2)
  call void @llvm.spv.value.md(metadata !10)
  call void (...) @llvm.fake.use(i32 %3)
  call void @llvm.spv.store.i32.p4(i32 %3, ptr addrspace(4) %2, i16 2, i8 8)
  ret void
}

With expensive checks enabled the test is passing still, even though I don't think this is valid IR, since we have:

%3 = call [8 x i8] (i32, ...) @llvm.spv.extractv.a8i8(i32 %1, i32 2)
call void (...) @llvm.fake.use(i32 %3)
call void @llvm.spv.store.i32.p4(i32 %3, ptr addrspace(4) %2, i16 2, i8 8)

Can we replace the call to llvm.spv.extractv.a8i8 with a call to llvm.spv.extractv.i32 and then mutate the type ?

}

auto *NewI = B.CreateIntrinsic(
Intrinsic::spv_store, {I.getValueOperand()->getType(), PtrOp->getType()},
{I.getValueOperand(), PtrOp, B.getInt16(Flags),
Expand Down
11 changes: 11 additions & 0 deletions llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3297,6 +3297,17 @@ bool SPIRVInstructionSelector::selectInsertVal(Register ResVReg,
bool SPIRVInstructionSelector::selectExtractVal(Register ResVReg,
const SPIRVType *ResType,
MachineInstr &I) const {
Type *MaybeResTy = nullptr;
StringRef ResName;
if (GR.findValueAttrs(&I, MaybeResTy, ResName) &&
MaybeResTy != GR.getTypeForSPIRVType(ResType)) {
assert(!MaybeResTy ||
MaybeResTy->isAggregateType() &&
"Expected aggregate type for extractv instruction");
ResType = GR.getOrCreateSPIRVType(MaybeResTy, I,
SPIRV::AccessQualifier::ReadWrite, false);
GR.assignSPIRVTypeToVReg(ResType, ResVReg, *I.getMF());
}
MachineBasicBlock &BB = *I.getParent();
auto MIB = BuildMI(BB, I, I.getDebugLoc(), TII.get(SPIRV::OpCompositeExtract))
.addDef(ResVReg)
Expand Down
30 changes: 30 additions & 0 deletions llvm/test/CodeGen/SPIRV/instructions/nested-composites.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,19 @@
; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s
; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s
; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv32-unknown-unknown %s -o - -filetype=obj | spirv-val %}
; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -filetype=obj | spirv-val %}

; CHECK-DAG: OpName [[FOOBAR:%.+]] "foobar"
; CHECK-DAG: OpName [[PRODUCER:%.+]] "producer"
; CHECK-DAG: OpName [[CONSUMER:%.+]] "consumer"
; CHECK-DAG: OpName [[SNEAKY:%.+]] "sneaky"
; CHECK-DAG: OpName [[ARR_RET:%.+]] "arr_ret"

; CHECK-NOT: DAG-FENCE

%ty1 = type {i16, i32}
%ty2 = type {%ty1, i64}
%ty3 = type {ptr addrspace(4), ptr addrspace(4), [8 x i8]}

; CHECK-DAG: [[I16:%.+]] = OpTypeInt 16
; CHECK-DAG: [[I32:%.+]] = OpTypeInt 32
Expand All @@ -18,6 +24,12 @@
; CHECK-DAG: [[UNDEF_I64:%.+]] = OpUndef [[I64]]
; CHECK-DAG: [[UNDEF_TY2:%.+]] = OpUndef [[TY2]]
; CHECK-DAG: [[CST_42:%.+]] = OpConstant [[I32]] 42
; CHECK-DAG: [[I8:%.+]] = OpTypeInt 8 0
; CHECK-DAG: [[PTR:%.+]] = OpTypePointer Generic [[I8]]
; CHECK-DAG: [[CST_8:%.+]] = OpConstant [[I32]] 8
; CHECK-DAG: [[ARR:%.+]] = OpTypeArray [[I8]] [[CST_8]]
; CHECK-DAG: [[TY3:%.+]] = OpTypeStruct [[PTR]] [[PTR]] [[ARR]]
; CHECK-DAG: [[PTR_ARR:%.+]] = OpTypePointer Generic [[ARR]]

; CHECK-NOT: DAG-FENCE

Expand Down Expand Up @@ -62,3 +74,21 @@ define i32 @consumer(%ty2 %agg) {
; CHECK: [[RET:%.+]] = OpCompositeExtract [[I32]] [[AGG]] 0 1
; CHECK: OpReturnValue [[RET]]
; CHECK: OpFunctionEnd

declare %ty3 @arr_ret()

define void @sneaky(ptr addrspace(4) %p, [8 x i8] %a) {
Copy link
Contributor

@jmmartinez jmmartinez Dec 16, 2025

Choose a reason for hiding this comment

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

Is %a needed to trigger the bug ?

%1 = call spir_func %ty3 @arr_ret()
%2 = getelementptr inbounds nuw %ty3, ptr addrspace(4) %p, i32 0, i32 2
%3 = extractvalue %ty3 %1, 2
store [8 x i8] %3, ptr addrspace(4) %2, align 8
ret void
}

; CHECK: [[SNEAKY]] = OpFunction
; CHECK: [[PTR_AGG:%.+]] = OpFunctionParameter
; CHECK: [[A:%.+]] = OpFunctionParameter [[ARR]]
; CHECK: [[AGG_RET:%.+]] = OpFunctionCall [[TY3]]
; CHECK: [[GEP:%.+]] = OpInBoundsPtrAccessChain [[PTR_ARR]] [[PTR_AGG]]
; CHECK: [[EXTRACTV:%.+]] = OpCompositeExtract [[ARR]] [[AGG_RET]] 2
; CHECK: OpStore [[GEP]] [[EXTRACTV]]