[SPIR-V] Fix translation of aggregate undef operands

[mpaszkowski]

Summary

This patch:

• Extends the preprocessing of constant aggregates for undefs. Such operands are replaced with a call to llvm.spv.const.composite.
• Adds a test undef-composite-store.ll demonstrating the issue.
• Adjusts and fixes formatting of two other tests.

More about this patch can be found here.

[mpaszkowski]

Gentle ping!

[arsenm]

llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp

179–180

this dyn_cast to constant doesn't make much sense to me, either you don't need this second cast or you should dyn_cast to UndefValue?

185

I'm guessing this doesn't handle nested structs properly

llvm/test/CodeGen/SPIRV/instructions/undef-composite-store.ll

16

Needs some tests with arrays, nested struct / array / vector combinations

[iliya-diyachkov]

Checking (by spirv-val) the backend's binary output for undef-composite-store.ll test shows this issue:

error: line 25: OpStore Pointer <id> '2[%ptr]'s type does not match Object <id> '13[%13]'s type.
  OpStore %ptr %13 Aligned 4

I would also suggest not changing the current Undef output format (implied in the two changed tests), but generating the output closer to the translator's one (despite the fact that the implemented solution looks standard-compliant). I suppose this may be implemented by introducing a new spv_undef intrinsic (similar to spv_alloca).

[mpaszkowski]

Thanks @arsenm and @iliya-diyachkov for your comments! I have created a new revision https://reviews.llvm.org/D143107 which provides a solution that is compatible with the Khronos SPIR-V Translator and should support nested aggregates. I will close this one.

llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp

for (const auto &Op : I->operands()) {
         I->replaceUsesOfWith(Op, CCI);
         KeepInst = true;
         AggrConsts[CCI] = AggrC;
       };
 
       if (auto *AggrC = dyn_cast<ConstantAggregate>(Op)) {
         SmallVector<Value *> Args(AggrC->op_begin(), AggrC->op_end());
         BuildCompositeIntrinsic(AggrC, Args);
+      } else if (isa<UndefValue>(Op) && Op->getType()->isAggregateType()) {
+        auto *AggrC = dyn_cast<Constant>(Op);
+        SmallVector<Value *> Args;
+        unsigned Idx = 0;
+        while (auto Elt = AggrC->getAggregateElement(Idx++))
+          Args.push_back(Elt);
+        BuildCompositeIntrinsic(AggrC, Args);
       } else if (auto *AggrC = dyn_cast<ConstantDataArray>(Op)) {
         SmallVector<Value *> Args;
         for (unsigned i = 0; i < AggrC->getNumElements(); ++i)
           Args.push_back(AggrC->getElementAsConstant(i));
         BuildCompositeIntrinsic(AggrC, Args);
       } else if (isa<ConstantAggregateZero>(Op) &&
                  !Op->getType()->isVectorTy()) {
         auto *AggrC = cast<ConstantAggregateZero>(Op);

llvm/test/CodeGen/SPIRV/instructions/call-complex-function.ll

 ; RUN: llc -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s
 
-; CHECK-DAG: OpName [[FUN:%.+]] "fun"
-; CHECK-DAG: OpName [[FOO:%.+]] "foo"
-; CHECK-DAG: OpName [[GOO:%.+]] "goo"
+; CHECK-DAG: OpName %[[#FUN:]] "fun"
+; CHECK-DAG: OpName %[[#FOO:]] "foo"
+; CHECK-DAG: OpName %[[#GOO:]] "goo"
 
 ; CHECK-NOT: DAG-FENCE
 
-; CHECK-DAG: [[I16:%.+]] = OpTypeInt 16
-; CHECK-DAG: [[I32:%.+]] = OpTypeInt 32
-; CHECK-DAG: [[I64:%.+]] = OpTypeInt 64
-; CHECK-DAG: [[FN3:%.+]] = OpTypeFunction [[I32]] [[I32]] [[I16]] [[I64]]
-; CHECK-DAG: [[PAIR:%.+]] = OpTypeStruct [[I32]] [[I16]]
-; CHECK-DAG: [[FN1:%.+]] = OpTypeFunction [[I32]] [[I32]]
-; CHECK-DAG: [[FN2:%.+]] = OpTypeFunction [[I32]] [[PAIR]] [[I64]]
+; CHECK-DAG: %[[#I16:]] = OpTypeInt 16
+; CHECK-DAG: %[[#I32:]] = OpTypeInt 32
+; CHECK-DAG: %[[#I64:]] = OpTypeInt 64
+; CHECK-DAG: %[[#FN3:]] = OpTypeFunction %[[#I32]] %[[#I32]] %[[#I16]] %[[#I64]]
+; CHECK-DAG: %[[#PAIR:]] = OpTypeStruct %[[#I32]] %[[#I16]]
+; CHECK-DAG: %[[#FN1:]] = OpTypeFunction %[[#I32]] %[[#I32]]
+; CHECK-DAG: %[[#FN2:]] = OpTypeFunction %[[#I32]] %[[#PAIR]] %[[#I64]]
 ;; According to the Specification, the OpUndef can be defined in Function.
 ;; But the Specification also recommends defining it here. So we enforce that.
-; CHECK-DAG: [[UNDEF:%.+]] = OpUndef [[PAIR]]
+; CHECK-DAG: %[[#UNDEF1:]] = OpUndef %[[#I32]]
+; CHECK-DAG: %[[#UNDEF2:]] = OpUndef %[[#I16]]
+; CHECK-DAG: %[[#COMPOSITE:]] = OpConstantComposite %[[#PAIR]] %[[#UNDEF1]] %[[#UNDEF2]]
 
 
 declare i32 @fun(i32 %value)
 
 ;; Check for @fun declaration
-; CHECK:      [[FUN]] = OpFunction [[I32]] None [[FN1]]
-; CHECK-NEXT: OpFunctionParameter [[I32]]
+; CHECK:      %[[#FUN]] = OpFunction %[[#I32]] None %[[#FN1]]
+; CHECK-NEXT: OpFunctionParameter %[[#I32]]
 ; CHECK-NEXT: OpFunctionEnd
 
 
 define i32 @foo({i32, i16} %in, i64 %unused) {
   %first = extractvalue {i32, i16} %in, 0
   %bar = call i32 @fun(i32 %first)
   ret i32 %bar
 }
 
-; CHECK:      [[GOO]] = OpFunction [[I32]] None [[FN3]]
-; CHECK-NEXT: [[A:%.+]] = OpFunctionParameter [[I32]]
-; CHECK-NEXT: [[B:%.+]] = OpFunctionParameter [[I16]]
-; CHECK-NEXT: [[C:%.+]] = OpFunctionParameter [[I64]]
-; CHECK:      [[AGG1:%.+]] = OpCompositeInsert [[PAIR]] [[A]] [[UNDEF]] 0
-; CHECK:      [[AGG2:%.+]] = OpCompositeInsert [[PAIR]] [[B]] [[AGG1]] 1
-; CHECK:      [[RET:%.+]] = OpFunctionCall [[I32]] [[FOO]] [[AGG2]] [[C]]
-; CHECK:      OpReturnValue [[RET]]
+; CHECK:      %[[#GOO]] = OpFunction %[[#I32]] None %[[#FN3]]
+; CHECK-NEXT: %[[#A:]] = OpFunctionParameter %[[#I32]]
+; CHECK-NEXT: %[[#B:]] = OpFunctionParameter %[[#I16]]
+; CHECK-NEXT: %[[#C:]] = OpFunctionParameter %[[#I64]]
+; CHECK:      %[[#AGG1:]] = OpCompositeInsert %[[#PAIR]] %[[#A]] %[[#COMPOSITE]] 0
+; CHECK:      %[[#AGG2:]] = OpCompositeInsert %[[#PAIR]] %[[#B]] %[[#AGG1]] 1
+; CHECK:      %[[#RET:]] = OpFunctionCall %[[#I32]] %[[#FOO]] %[[#AGG2]] %[[#C]]
+; CHECK:      OpReturnValue %[[#RET]]
 ; CHECK:      OpFunctionEnd
 
-; CHECK:      [[FOO]] = OpFunction [[I32]] None [[FN2]]
-; CHECK-NEXT: [[IN:%.+]] = OpFunctionParameter [[PAIR]]
-; CHECK-NEXT: OpFunctionParameter [[I64]]
-; CHECK:      [[FIRST:%.+]] = OpCompositeExtract [[I32]] [[IN]] 0
-; CHECK:      [[BAR:%.+]] = OpFunctionCall [[I32]] [[FUN]] [[FIRST]]
-; CHECK:      OpReturnValue [[BAR]]
+; CHECK:      %[[#FOO]] = OpFunction %[[#I32]] None %[[#FN2]]
+; CHECK-NEXT: %[[#IN:]] = OpFunctionParameter %[[#PAIR]]
+; CHECK-NEXT: OpFunctionParameter %[[#I64]]
+; CHECK:      %[[#FIRST:]] = OpCompositeExtract %[[#I32]] %[[#IN]] 0
+; CHECK:      %[[#BAR:]] = OpFunctionCall %[[#I32]] %[[#FUN]] %[[#FIRST]]
+; CHECK:      OpReturnValue %[[#BAR]]
 ; CHECK:      OpFunctionEnd
 
 define i32 @goo(i32 %a, i16 %b, i64 %c) {
   %agg1 = insertvalue {i32, i16} undef, i32 %a, 0
   %agg2 = insertvalue {i32, i16} %agg1, i16 %b, 1
   %ret = call i32 @foo({i32, i16} %agg2, i64 %c)
   ret i32 %ret
 }
 
 ;; TODO: test tailcall?

llvm/test/CodeGen/SPIRV/instructions/nested-composites.ll

 ; RUN: llc -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s
 
-; CHECK-DAG: OpName [[FOOBAR:%.+]] "foobar"
-; CHECK-DAG: OpName [[PRODUCER:%.+]] "producer"
-; CHECK-DAG: OpName [[CONSUMER:%.+]] "consumer"
+; CHECK-DAG: OpName %[[#FOOBAR:]] "foobar"
+; CHECK-DAG: OpName %[[#PRODUCER:]] "producer"
+; CHECK-DAG: OpName %[[#CONSUMER:]] "consumer"
 
 ; CHECK-NOT: DAG-FENCE
 
 %ty1 = type {i16, i32}
 %ty2 = type {%ty1, i64}
 
-; CHECK-DAG: [[I16:%.+]] = OpTypeInt 16
-; CHECK-DAG: [[I32:%.+]] = OpTypeInt 32
-; CHECK-DAG: [[I64:%.+]] = OpTypeInt 64
-; CHECK-DAG: [[TY1:%.+]] = OpTypeStruct [[I16]] [[I32]]
-; CHECK-DAG: [[TY2:%.+]] = OpTypeStruct [[TY1]] [[I64]]
-; CHECK-DAG: [[UNDEF_I16:%.+]] = OpUndef [[I16]]
-; CHECK-DAG: [[UNDEF_I64:%.+]] = OpUndef [[I64]]
-; CHECK-DAG: [[UNDEF_TY2:%.+]] = OpUndef [[TY2]]
-; CHECK-DAG: [[CST_42:%.+]] = OpConstant [[I32]] 42
+; CHECK-DAG: %[[#I16:]] = OpTypeInt 16
+; CHECK-DAG: %[[#I32:]] = OpTypeInt 32
+; CHECK-DAG: %[[#I64:]] = OpTypeInt 64
+; CHECK-DAG: %[[#TY1:]] = OpTypeStruct %[[#I16]] %[[#I32]]
+; CHECK-DAG: %[[#TY2:]] = OpTypeStruct %[[#TY1]] %[[#I64]]
+; CHECK-DAG: %[[#UNDEF_I16:]] = OpUndef %[[#I16]]
+; CHECK-DAG: %[[#UNDEF_I64:]] = OpUndef %[[#I64]]
+; CHECK-DAG: %[[#UNDEF_I32:]] = OpUndef %[[#I32]]
+; CHECK-DAG: %[[#CST_42:]] = OpConstant %[[#I32]] 42
+; CHECK-DAG: %[[#COMPOSITE1:]] = OpConstantComposite %[[#TY1]] %[[#UNDEF_I16]] %[[#UNDEF_I32]]
+; CHECK-DAG: %[[#COMPOSITE2:]] = OpConstantComposite %[[#TY2]] %[[#COMPOSITE1]] %[[#UNDEF_I64]]
 
 ; CHECK-NOT: DAG-FENCE
 
 define i32 @foobar() {
   %agg = call %ty2 @producer(i16 undef, i32 42, i64 undef)
   %ret = call i32 @consumer(%ty2 %agg)
   ret i32 %ret
 }
 
-; CHECK: [[FOOBAR]] = OpFunction
-; CHECK: [[AGG:%.+]] = OpFunctionCall [[TY2]] [[PRODUCER]] [[UNDEF_I16]] [[CST_42]] [[UNDEF_I64]]
-; CHECK: [[RET:%.+]] = OpFunctionCall [[I32]] [[CONSUMER]] [[AGG]]
-; CHECK: OpReturnValue [[RET]]
+; CHECK: %[[#FOOBAR]] = OpFunction
+; CHECK: %[[#AGG:]] = OpFunctionCall %[[#TY2]] %[[#PRODUCER]] %[[#UNDEF_I16]] %[[#CST_42]] %[[#UNDEF_I64]]
+; CHECK: %[[#RET:]] = OpFunctionCall %[[#I32]] %[[#CONSUMER]] %[[#AGG]]
+; CHECK: OpReturnValue %[[#RET]]
 ; CHECK: OpFunctionEnd
 
 
 define %ty2 @producer(i16 %a, i32 %b, i64 %c) {
   %agg1 = insertvalue %ty2 undef, i16 %a, 0, 0
   %agg2 = insertvalue %ty2 %agg1, i32 %b, 0, 1
   %agg3 = insertvalue %ty2 %agg2, i64 %c, 1
   ret %ty2 %agg3
 }
 
-; CHECK: [[PRODUCER]] = OpFunction
-; CHECK: [[A:%.+]] = OpFunctionParameter [[I16]]
-; CHECK: [[B:%.+]] = OpFunctionParameter [[I32]]
-; CHECK: [[C:%.+]] = OpFunctionParameter [[I64]]
-; CHECK: [[AGG1:%.+]] = OpCompositeInsert [[TY2]] [[A]] [[UNDEF_TY2]] 0 0
-; CHECK: [[AGG2:%.+]] = OpCompositeInsert [[TY2]] [[B]] [[AGG1]] 0 1
-; CHECK: [[AGG3:%.+]] = OpCompositeInsert [[TY2]] [[C]] [[AGG2]] 1
-; CHECK: OpReturnValue [[AGG3]]
+; CHECK: %[[#PRODUCER]] = OpFunction
+; CHECK: %[[#A:]] = OpFunctionParameter %[[#I16]]
+; CHECK: %[[#B:]] = OpFunctionParameter %[[#I32]]
+; CHECK: %[[#C:]] = OpFunctionParameter %[[#I64]]
+; CHECK: %[[#AGG1:]] = OpCompositeInsert %[[#TY2]] %[[#A]] %[[#COMPOSITE2]] 0 0
+; CHECK: %[[#AGG2:]] = OpCompositeInsert %[[#TY2]] %[[#B]] %[[#AGG1]] 0 1
+; CHECK: %[[#AGG3:]] = OpCompositeInsert %[[#TY2]] %[[#C]] %[[#AGG2]] 1
+; CHECK: OpReturnValue %[[#AGG3]]
 ; CHECK: OpFunctionEnd
 
 
 define i32 @consumer(%ty2 %agg) {
   %ret = extractvalue %ty2 %agg, 0, 1
   ret i32 %ret
 }
 
-; CHECK: [[CONSUMER]] = OpFunction
-; CHECK: [[AGG:%.+]] = OpFunctionParameter [[TY2]]
-; CHECK: [[RET:%.+]] = OpCompositeExtract [[I32]] [[AGG]] 0 1
-; CHECK: OpReturnValue [[RET]]
+; CHECK: %[[#CONSUMER]] = OpFunction
+; CHECK: %[[#AGG:]] = OpFunctionParameter %[[#TY2]]
+; CHECK: %[[#RET:]] = OpCompositeExtract %[[#I32]] %[[#AGG]] 0 1
+; CHECK: OpReturnValue %[[#RET]]
 ; CHECK: OpFunctionEnd

llvm/test/CodeGen/SPIRV/instructions/undef-composite-store.ll

+; RUN: llc -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s
+
+; CHECK-DAG: %[[#I32:]] = OpTypeInt 32
+; CHECK-DAG: %[[#I16:]] = OpTypeInt 16
+; CHECK-DAG: %[[#STRUCT:]] = OpTypeStruct %[[#I32]] %[[#I16]]
+; CHECK-DAG: %[[#UNDEF_I32:]] = OpUndef %[[#I32]]
+; CHECK-DAG: %[[#UNDEF_I16:]] = OpUndef %[[#I16]]
+; CHECK-DAG: %[[#COMPOSITE:]] = OpConstantComposite %[[#STRUCT]] %[[#UNDEF_I32]] %[[#UNDEF_I16]]
+
+; CHECK: %[[#]] = OpFunction %[[#]] None %[[#]]
+; CHECK-NEXT: %[[#PTR:]] = OpFunctionParameter %[[#]]
+; CHECK-NEXT: %[[#]] = OpLabel
+; CHECK-NEXT: OpStore %[[#PTR]] %[[#COMPOSITE]] Aligned 4
+; CHECK-NEXT: OpReturn
+; CHECK-NEXT: OpFunctionEnd
+
+define void @foo(ptr %ptr) {
+  store { i32, i16 } undef, ptr %ptr
+  ret void
+}