Disallow structs, arrays, and overwide vectors from typed buffers (microsoft#7130)

These never reliably generated the right results, so they are being
disallowed. The check takes place in SemaHLSL and replaces a few places
that checked for such things in special cases. To facilitate the check,
the definition of the texture bit was expanded to include RW textures.
This is consistent with the description and since the bit was never
actually used before, has no other consequences.

Some code that executed after Sema that either produced errors or
processed resources with these now forbidden types was removed. Most of
this is from CodeGen and HLOperationLower where code was generated or
expanded for load/store operations on such resources.

Additionally moved the check for overlarge elements in typed resources
to enable keeping some tests around that tested these side by side as
well as adding a new one. I changed the nature of the check a bit to
limit the types to four elements and also that they be no larger than 4
32-bit elements. This means that 64-bit elements are limited to two.
While there wasn't an error that caught this before, there was an assert
that fired.

Incidentally fixes an issue with error reporting for modified types
after the first mismatch of that intrinsic is encountered. Because the
insertion code changed them in the temp array and the stored function
type, but printed from the temp array and the later encounters didn't
add the modifiers to the temp array, later errors would incorrectly
leave off the reference. By retrieving the type from the function type,
the reference is always preserved.

This required changing a fair number of tests that relied on this
behavior including removing test cases and entire test files that
existed solely for such tests.

Fixes microsoft#7080

Author: Greg Roth

docs/ReleaseNotes.md

@@ -21,6 +21,8 @@ The included licenses apply to the following files:
 
 Place release notes for the upcoming release below this line and remove this line upon naming this release.
 
+- Typed buffers (including ROV buffers) no longer accept types other than vectors and scalars. Any other types will produce descriptive errors. This removes support for appropriately sized matrices and structs. Though it worked in some contexts, code generated from such types was unreliable.
+
 ### Version 1.8.2502
 
 This cumulative release contains numerous bug fixes and stability improvements.

lib/HLSL/HLOperationLower.cpp

@@ -7829,84 +7829,6 @@ void TranslateCBOperationsLegacy(Value *handle, Value *ptr, OP *hlslOP,
 
 // Structured buffer.
 namespace {
-// Load a value from a typedef buffer with an offset.
-// Typed buffer do not directly support reading at offsets
-// because the whole value (e.g. float4) must be read at once.
-// If we are provided a non-zero offset, we need to simulate it
-// by returning the correct elements.
-using ResRetValueArray = std::array<Value *, 4>;
-static ResRetValueArray
-GenerateTypedBufferLoad(Value *Handle, Type *BufferElemTy, Value *ElemIdx,
-                        Value *StatusPtr, OP *HlslOP, IRBuilder<> &Builder) {
-
-  OP::OpCode OpCode = OP::OpCode::BufferLoad;
-  Value *LoadArgs[] = {HlslOP->GetU32Const((unsigned)OpCode), Handle, ElemIdx,
-                       UndefValue::get(Builder.getInt32Ty())};
-  Function *LoadFunc = HlslOP->GetOpFunc(OpCode, BufferElemTy);
-  Value *Load =
-      Builder.CreateCall(LoadFunc, LoadArgs, OP::GetOpCodeName(OpCode));
-
-  ResRetValueArray ResultValues;
-  for (unsigned i = 0; i < ResultValues.size(); ++i) {
-    ResultValues[i] =
-        cast<ExtractValueInst>(Builder.CreateExtractValue(Load, {i}));
-  }
-
-  UpdateStatus(Load, StatusPtr, Builder, HlslOP);
-  return ResultValues;
-}
-
-static AllocaInst *SpillValuesToArrayAlloca(ArrayRef<Value *> Values,
-                                            IRBuilder<> &Builder) {
-  DXASSERT_NOMSG(!Values.empty());
-  IRBuilder<> AllocaBuilder(
-      dxilutil::FindAllocaInsertionPt(Builder.GetInsertPoint()));
-  AllocaInst *ArrayAlloca = AllocaBuilder.CreateAlloca(
-      ArrayType::get(Values[0]->getType(), Values.size()));
-  for (unsigned i = 0; i < Values.size(); ++i) {
-    Value *ArrayElemPtr = Builder.CreateGEP(
-        ArrayAlloca, {Builder.getInt32(0), Builder.getInt32(i)});
-    Builder.CreateStore(Values[i], ArrayElemPtr);
-  }
-  return ArrayAlloca;
-}
-
-static Value *ExtractFromTypedBufferLoad(const ResRetValueArray &ResRet,
-                                         Type *ResultTy, Value *Offset,
-                                         IRBuilder<> &Builder) {
-  unsigned ElemCount =
-      ResultTy->isVectorTy() ? ResultTy->getVectorNumElements() : 1;
-  DXASSERT_NOMSG(ElemCount < ResRet.size());
-  unsigned ElemSizeInBytes = ResRet[0]->getType()->getScalarSizeInBits() / 8;
-
-  SmallVector<Value *, 4> Elems;
-  if (ConstantInt *OffsetAsConstantInt = dyn_cast<ConstantInt>(Offset)) {
-    // Get all elements to be returned
-    uint64_t FirstElemOffset = OffsetAsConstantInt->getLimitedValue();
-    DXASSERT_NOMSG(FirstElemOffset % ElemSizeInBytes == 0);
-    uint64_t FirstElemIdx = FirstElemOffset / ElemSizeInBytes;
-    DXASSERT_NOMSG(FirstElemIdx <= ResRet.size() - ElemCount);
-    for (unsigned ElemIdx = 0; ElemIdx < ElemCount; ++ElemIdx) {
-      Elems.emplace_back(
-          ResRet[std::min<size_t>(FirstElemIdx + ElemIdx, ResRet.size() - 1)]);
-    }
-  } else {
-    Value *ArrayAlloca = SpillValuesToArrayAlloca(
-        ArrayRef<Value *>(ResRet.data(), ResRet.size()), Builder);
-
-    // Get all elements to be returned through dynamic indices
-    Value *FirstElemIdx =
-        Builder.CreateUDiv(Offset, Builder.getInt32(ElemSizeInBytes));
-    for (unsigned i = 0; i < ElemCount; ++i) {
-      Value *ElemIdx = Builder.CreateAdd(FirstElemIdx, Builder.getInt32(i));
-      Value *ElemPtr =
-          Builder.CreateGEP(ArrayAlloca, {Builder.getInt32(0), ElemIdx});
-      Elems.emplace_back(Builder.CreateLoad(ElemPtr));
-    }
-  }
-
-  return ScalarizeElements(ResultTy, Elems, Builder);
-}
 
 Value *GenerateRawBufLd(Value *handle, Value *bufIdx, Value *offset,
                         Value *status, Type *EltTy,
@@ -8378,27 +8300,16 @@ void TranslateStructBufSubscriptUser(Instruction *user, Value *handle,
 
     if (LdInst) {
       unsigned NumComponents = 0;
-      Value *NewLd = nullptr;
       if (VectorType *VTy = dyn_cast<VectorType>(Ty))
         NumComponents = VTy->getNumElements();
       else
         NumComponents = 1;
-
-      if (ResKind == HLResource::Kind::TypedBuffer) {
-        // Typed buffer cannot have offsets, they must be loaded all at once
-        ResRetValueArray ResRet = GenerateTypedBufferLoad(
-            handle, pOverloadTy, bufIdx, status, OP, Builder);
-
-        NewLd = ExtractFromTypedBufferLoad(ResRet, Ty, Offset, Builder);
-      } else {
-        Value *ResultElts[4];
-        Constant *Alignment =
-            OP->GetI32Const(DL.getTypeAllocSize(Ty->getScalarType()));
-        GenerateRawBufLd(handle, bufIdx, Offset, status, pOverloadTy,
-                         ResultElts, OP, Builder, NumComponents, Alignment);
-        NewLd = ScalarizeElements(Ty, ResultElts, Builder);
-      }
-
+      Value *ResultElts[4];
+      Constant *Alignment =
+          OP->GetI32Const(DL.getTypeAllocSize(Ty->getScalarType()));
+      GenerateRawBufLd(handle, bufIdx, Offset, status, pOverloadTy, ResultElts,
+                       OP, Builder, NumComponents, Alignment);
+      Value *NewLd = ScalarizeElements(Ty, ResultElts, Builder);
       LdInst->replaceAllUsesWith(NewLd);
     } else {
       Value *val = StInst->getValueOperand();
@@ -8821,66 +8732,12 @@ void TranslateHLSubscript(CallInst *CI, HLSubscriptOpcode opcode,
       RetTy = ObjTy->getStructElementType(0);
       Translated = true;
 
-      if (DXIL::IsStructuredBuffer(RK)) {
-        TranslateStructBufSubscript(CI, handle, /*status*/ nullptr, hlslOP, RK,
-                                    helper.dataLayout);
-      } else if (RetTy->isAggregateType() &&
-                 RK == DxilResource::Kind::TypedBuffer) {
-
+      if (DXIL::IsStructuredBuffer(RK))
         TranslateStructBufSubscript(CI, handle, /*status*/ nullptr, hlslOP, RK,
                                     helper.dataLayout);
-        // Clear offset for typed buf.
-        for (auto User = handle->user_begin(); User != handle->user_end();) {
-          CallInst *CI = cast<CallInst>(*(User++));
-          // Skip not lowered HL functions.
-          if (hlsl::GetHLOpcodeGroupByName(CI->getCalledFunction()) !=
-              HLOpcodeGroup::NotHL)
-            continue;
-          switch (hlslOP->GetDxilOpFuncCallInst(CI)) {
-          case DXIL::OpCode::BufferLoad: {
-            CI->setArgOperand(DXIL::OperandIndex::kBufferLoadCoord1OpIdx,
-                              UndefValue::get(helper.i32Ty));
-          } break;
-          case DXIL::OpCode::BufferStore: {
-            CI->setArgOperand(DXIL::OperandIndex::kBufferStoreCoord1OpIdx,
-                              UndefValue::get(helper.i32Ty));
-          } break;
-          case DXIL::OpCode::AtomicBinOp: {
-            CI->setArgOperand(DXIL::OperandIndex::kAtomicBinOpCoord1OpIdx,
-                              UndefValue::get(helper.i32Ty));
-          } break;
-          case DXIL::OpCode::AtomicCompareExchange: {
-            CI->setArgOperand(DXIL::OperandIndex::kAtomicCmpExchangeCoord1OpIdx,
-                              UndefValue::get(helper.i32Ty));
-          } break;
-          case DXIL::OpCode::RawBufferLoad: {
-            // Structured buffer inside a typed buffer must be converted to
-            // typed buffer load. Typed buffer load is equivalent to raw buffer
-            // load, except there is no mask.
-            StructType *STy =
-                cast<StructType>(CI->getFunctionType()->getReturnType());
-            Type *ETy = STy->getElementType(0);
-            SmallVector<Value *, 4> Args;
-            Args.emplace_back(
-                hlslOP->GetI32Const((unsigned)DXIL::OpCode::BufferLoad));
-            Args.emplace_back(CI->getArgOperand(1));          // handle
-            Args.emplace_back(CI->getArgOperand(2));          // index
-            Args.emplace_back(UndefValue::get(helper.i32Ty)); // offset
-            IRBuilder<> builder(CI);
-            Function *newFunction =
-                hlslOP->GetOpFunc(DXIL::OpCode::BufferLoad, ETy);
-            CallInst *newCall = builder.CreateCall(newFunction, Args);
-            CI->replaceAllUsesWith(newCall);
-            CI->eraseFromParent();
-          } break;
-          default:
-            DXASSERT(0, "Invalid operation on resource handle");
-            break;
-          }
-        }
-      } else {
+      else
         TranslateDefaultSubscript(CI, helper, pObjHelper, Translated);
-      }
+
       return;
     }
   }

tools/clang/include/clang/AST/HlslTypes.h

@@ -370,12 +370,14 @@ void AddStdIsEqualImplementation(clang::ASTContext &context, clang::Sema &sema);
 clang::CXXRecordDecl *DeclareTemplateTypeWithHandle(
     clang::ASTContext &context, llvm::StringRef name,
     uint8_t templateArgCount = 1,
-    clang::TypeSourceInfo *defaultTypeArgValue = nullptr);
+    clang::TypeSourceInfo *defaultTypeArgValue = nullptr,
+    clang::InheritableAttr *Attr = nullptr);
 
 clang::CXXRecordDecl *DeclareTemplateTypeWithHandleInDeclContext(
     clang::ASTContext &context, clang::DeclContext *declContext,
     llvm::StringRef name, uint8_t templateArgCount,
-    clang::TypeSourceInfo *defaultTypeArgValue);
+    clang::TypeSourceInfo *defaultTypeArgValue,
+    clang::InheritableAttr *Attr = nullptr);
 
 clang::CXXRecordDecl *DeclareUIntTemplatedTypeWithHandle(
     clang::ASTContext &context, llvm::StringRef typeName,

tools/clang/include/clang/Basic/Attr.td

@@ -992,6 +992,13 @@ def HLSLNodeTrackRWInputSharing : InheritableAttr {
   let Documentation = [Undocumented];
 }
 
+def HLSLResource : InheritableAttr {
+  let Spellings = []; // No spellings!
+  let Args = [UnsignedArgument<"ResKind">, UnsignedArgument<"ResClass">];
+  let Subjects = SubjectList<[CXXRecord]>;
+  let Documentation = [Undocumented];
+}
+
 def HLSLNodeObject : InheritableAttr {
   let Spellings = []; // No spellings!
   let Subjects = SubjectList<[CXXRecord]>;

tools/clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -7596,6 +7596,10 @@ def err_hlsl_unsupported_buffer_packoffset : Error<
   "packoffset is only allowed within a constant buffer, not on the constant buffer declaration">;
 def err_hlsl_unsupported_buffer_slot_target_specific : Error<
   "user defined constant buffer slots cannot be target specific">;
+def err_hlsl_unsupported_typedbuffer_template_parameter : Error<
+  "elements of typed buffers and textures must be scalars or vectors">;
+def err_hlsl_unsupported_typedbuffer_template_parameter_size : Error<
+  "elements of typed buffers and textures must fit in four 32-bit quantities">;
 def err_hlsl_unsupported_payload_access_qualifier : Error<
   "payload access qualifiers are only allowed for member variables of a payload structure">;
 def err_hlsl_unsupported_payload_access_qualifier_struct : Error<

tools/clang/lib/AST/ASTContextHLSL.cpp

@@ -903,18 +903,18 @@ void hlsl::AddStdIsEqualImplementation(clang::ASTContext &context,
 /// <parm name="templateArgCount">Number of template arguments (one or
 /// two).</param> <parm name="defaultTypeArgValue">If assigned, the default
 /// argument for the element template.</param>
-CXXRecordDecl *
-hlsl::DeclareTemplateTypeWithHandle(ASTContext &context, StringRef name,
-                                    uint8_t templateArgCount,
-                                    TypeSourceInfo *defaultTypeArgValue) {
+CXXRecordDecl *hlsl::DeclareTemplateTypeWithHandle(
+    ASTContext &context, StringRef name, uint8_t templateArgCount,
+    TypeSourceInfo *defaultTypeArgValue, InheritableAttr *Attr) {
   return DeclareTemplateTypeWithHandleInDeclContext(
       context, context.getTranslationUnitDecl(), name, templateArgCount,
-      defaultTypeArgValue);
+      defaultTypeArgValue, Attr);
 }
 
 CXXRecordDecl *hlsl::DeclareTemplateTypeWithHandleInDeclContext(
     ASTContext &context, DeclContext *declContext, StringRef name,
-    uint8_t templateArgCount, TypeSourceInfo *defaultTypeArgValue) {
+    uint8_t templateArgCount, TypeSourceInfo *defaultTypeArgValue,
+    InheritableAttr *Attr) {
   DXASSERT(templateArgCount != 0,
            "otherwise caller should be creating a class or struct");
   DXASSERT(templateArgCount <= 2, "otherwise the function needs to be updated "
@@ -968,6 +968,9 @@ CXXRecordDecl *hlsl::DeclareTemplateTypeWithHandleInDeclContext(
 
   typeDeclBuilder.addField("h", elementType);
 
+  if (Attr)
+    typeDeclBuilder.getRecordDecl()->addAttr(Attr);
+
   return typeDeclBuilder.getRecordDecl();
 }
 

tools/clang/lib/CodeGen/CGHLSLMS.cpp

@@ -3375,48 +3375,6 @@ void CGMSHLSLRuntime::CreateSubobject(
   }
 }
 
-static void CollectScalarTypes(std::vector<QualType> &ScalarTys, QualType Ty) {
-  if (Ty->isRecordType()) {
-    if (hlsl::IsHLSLMatType(Ty)) {
-      QualType EltTy = hlsl::GetHLSLMatElementType(Ty);
-      unsigned row = 0;
-      unsigned col = 0;
-      hlsl::GetRowsAndCols(Ty, row, col);
-      unsigned size = col * row;
-      for (unsigned i = 0; i < size; i++) {
-        CollectScalarTypes(ScalarTys, EltTy);
-      }
-    } else if (hlsl::IsHLSLVecType(Ty)) {
-      QualType EltTy = hlsl::GetHLSLVecElementType(Ty);
-      unsigned row = 0;
-      unsigned col = 0;
-      hlsl::GetRowsAndColsForAny(Ty, row, col);
-      unsigned size = col;
-      for (unsigned i = 0; i < size; i++) {
-        CollectScalarTypes(ScalarTys, EltTy);
-      }
-    } else {
-      const RecordType *RT = Ty->getAs<RecordType>();
-      RecordDecl *RD = RT->getDecl();
-      for (FieldDecl *field : RD->fields())
-        CollectScalarTypes(ScalarTys, field->getType());
-    }
-  } else if (Ty->isArrayType()) {
-    const clang::ArrayType *AT = Ty->getAsArrayTypeUnsafe();
-    QualType EltTy = AT->getElementType();
-    // Set it to 5 for unsized array.
-    unsigned size = 5;
-    if (AT->isConstantArrayType()) {
-      size = cast<ConstantArrayType>(AT)->getSize().getLimitedValue();
-    }
-    for (unsigned i = 0; i < size; i++) {
-      CollectScalarTypes(ScalarTys, EltTy);
-    }
-  } else {
-    ScalarTys.emplace_back(Ty);
-  }
-}
-
 bool CGMSHLSLRuntime::SetUAVSRV(SourceLocation loc,
                                 hlsl::DxilResourceBase::Class resClass,
                                 DxilResource *hlslRes, QualType QualTy) {
@@ -3443,66 +3401,13 @@ bool CGMSHLSLRuntime::SetUAVSRV(SourceLocation loc,
     hlslRes->SetSampleCount(sampleCount);
   }
 
-  if (hlsl::DxilResource::IsAnyTexture(kind)) {
-    const ClassTemplateSpecializationDecl *templateDecl =
-        cast<ClassTemplateSpecializationDecl>(RD);
-    const clang::TemplateArgument &texelTyArg =
-        templateDecl->getTemplateArgs()[0];
-    llvm::Type *texelTy = CGM.getTypes().ConvertType(texelTyArg.getAsType());
-    if (!texelTy->isFloatingPointTy() && !texelTy->isIntegerTy() &&
-        !hlsl::IsHLSLVecType(texelTyArg.getAsType())) {
-      DiagnosticsEngine &Diags = CGM.getDiags();
-      unsigned DiagID = Diags.getCustomDiagID(
-          DiagnosticsEngine::Error,
-          "texture resource texel type must be scalar or vector");
-      Diags.Report(loc, DiagID);
-      return false;
-    }
-  }
-
   QualType resultTy = hlsl::GetHLSLResourceResultType(QualTy);
   if (kind != hlsl::DxilResource::Kind::StructuredBuffer &&
       !resultTy.isNull()) {
     QualType Ty = resultTy;
     QualType EltTy = Ty;
-    if (hlsl::IsHLSLVecType(Ty)) {
+    if (hlsl::IsHLSLVecType(Ty))
       EltTy = hlsl::GetHLSLVecElementType(Ty);
-    } else if (hlsl::IsHLSLMatType(Ty)) {
-      EltTy = hlsl::GetHLSLMatElementType(Ty);
-    } else if (hlsl::IsHLSLAggregateType(resultTy)) {
-      // Struct or array in a none-struct resource.
-      std::vector<QualType> ScalarTys;
-      CollectScalarTypes(ScalarTys, resultTy);
-      unsigned size = ScalarTys.size();
-      if (size == 0) {
-        DiagnosticsEngine &Diags = CGM.getDiags();
-        unsigned DiagID = Diags.getCustomDiagID(
-            DiagnosticsEngine::Error,
-            "object's templated type must have at least one element");
-        Diags.Report(loc, DiagID);
-        return false;
-      }
-      if (size > 4) {
-        DiagnosticsEngine &Diags = CGM.getDiags();
-        unsigned DiagID = Diags.getCustomDiagID(
-            DiagnosticsEngine::Error, "elements of typed buffers and textures "
-                                      "must fit in four 32-bit quantities");
-        Diags.Report(loc, DiagID);
-        return false;
-      }
-
-      EltTy = ScalarTys[0];
-      for (QualType ScalarTy : ScalarTys) {
-        if (ScalarTy != EltTy) {
-          DiagnosticsEngine &Diags = CGM.getDiags();
-          unsigned DiagID = Diags.getCustomDiagID(
-              DiagnosticsEngine::Error,
-              "all template type components must have the same type");
-          Diags.Report(loc, DiagID);
-          return false;
-        }
-      }
-    }
 
     bool bSNorm = false;
     bool bHasNormAttribute = hlsl::HasHLSLUNormSNorm(Ty, &bSNorm);