Removing !hal.descriptor_set/iree_hal_descriptor_set_t. (#10146)

It was never fully implemented and the combination of push descriptors and upcoming binding tables should be sufficient for our uses. Not a breaking change as the compiler had never emitted code using them. Progress on #10144.
iree-org · Aug 22, 2022 · 1750213 · 1750213
1 parent e33c64c
commit 1750213
Show file tree

Hide file tree

Showing 51 changed files with 31 additions and 1,087 deletions.
diff --git a/compiler/src/iree/compiler/Dialect/HAL/Conversion/HALToVM/ConvertCommandBufferOps.cpp b/compiler/src/iree/compiler/Dialect/HAL/Conversion/HALToVM/ConvertCommandBufferOps.cpp
@@ -153,10 +153,6 @@ void populateHALCommandBufferToVMPatterns(MLIRContext *context,
   patterns.insert<CommandBufferPushDescriptorSetOpConversion>(
       context, importSymbols, typeConverter,
       "hal.command_buffer.push_descriptor_set");
-  patterns.insert<
-      VMImportOpConversion<IREE::HAL::CommandBufferBindDescriptorSetOp>>(
-      context, importSymbols, typeConverter,
-      "hal.command_buffer.bind_descriptor_set");
   patterns.insert<VMImportOpConversion<IREE::HAL::CommandBufferDispatchOp>>(
       context, importSymbols, typeConverter, "hal.command_buffer.dispatch");
   patterns

diff --git a/compiler/src/iree/compiler/Dialect/HAL/Conversion/HALToVM/test/command_buffer_ops.mlir b/compiler/src/iree/compiler/Dialect/HAL/Conversion/HALToVM/test/command_buffer_ops.mlir
@@ -107,28 +107,6 @@ func.func @command_buffer_copy_buffer(
 
 // -----
 
-// CHECK-LABEL: @command_buffer_bind_descriptor_set
-func.func @command_buffer_bind_descriptor_set(
-  %arg0: !hal.command_buffer,
-  %arg1: !hal.executable_layout,
-  %arg2: !hal.descriptor_set
-) {
-  %c0 = arith.constant 0 : index
-  %c100 = arith.constant 100 : index
-  // CHECK: vm.call.variadic @hal.command_buffer.bind_descriptor_set(%arg0, %arg1, %zero, %arg2, []) : (!vm.ref<!hal.command_buffer>, !vm.ref<!hal.executable_layout>, i32, !vm.ref<!hal.descriptor_set>, i64 ...)
-  hal.command_buffer.bind_descriptor_set<%arg0 : !hal.command_buffer>
-      layout(%arg1 : !hal.executable_layout)[%c0]
-      set(%arg2 : !hal.descriptor_set)
-  // CHECK: vm.call.variadic @hal.command_buffer.bind_descriptor_set(%arg0, %arg1, %zero, %arg2, [%c100]) : (!vm.ref<!hal.command_buffer>, !vm.ref<!hal.executable_layout>, i32, !vm.ref<!hal.descriptor_set>, i64 ...)
-  hal.command_buffer.bind_descriptor_set<%arg0 : !hal.command_buffer>
-      layout(%arg1 : !hal.executable_layout)[%c0]
-      set(%arg2 : !hal.descriptor_set)
-      offsets([%c100])
-  return
-}
-
-// -----
-
 // CHECK-LABEL: @command_buffer_dispatch
 func.func @command_buffer_dispatch(
   %arg0: !hal.command_buffer,

diff --git a/compiler/src/iree/compiler/Dialect/HAL/IR/HALBase.td b/compiler/src/iree/compiler/Dialect/HAL/IR/HALBase.td
@@ -257,16 +257,6 @@ def HAL_CommandBuffer : DialectType<
   let builderCall = "$_builder.getType<IREE::HAL::CommandBufferType>()";
 }
 
-def HAL_DescriptorSet : DialectType<
-    HAL_Dialect,
-    CPred<"$_self.isa<IREE::HAL::DescriptorSetType>()">,
-    "descriptor_set"> {
-  let description = [{
-    Descriptor set.
-  }];
-  let builderCall = "$_builder.getType<IREE::HAL::DescriptorSetType>()";
-}
-
 def HAL_DescriptorSetLayout : DialectType<
     HAL_Dialect,
     CPred<"$_self.isa<IREE::HAL::DescriptorSetLayoutType>()">,
@@ -359,7 +349,6 @@ def HAL_ObjectType : AnyTypeOf<[
   HAL_Buffer,
   HAL_BufferView,
   HAL_CommandBuffer,
-  HAL_DescriptorSet,
   HAL_DescriptorSetLayout,
   HAL_Device,
   HAL_Event,

diff --git a/compiler/src/iree/compiler/Dialect/HAL/IR/HALOps.cpp b/compiler/src/iree/compiler/Dialect/HAL/IR/HALOps.cpp
@@ -476,35 +476,6 @@ void CommandBufferPushDescriptorSetOp::build(
   state.addOperands(bindingLengths);
 }
 
-//===----------------------------------------------------------------------===//
-// hal.descriptor_set.create
-//===----------------------------------------------------------------------===//
-
-void DescriptorSetCreateOp::build(
-    OpBuilder &builder, OperationState &state, Value device, Value setLayout,
-    ArrayRef<DescriptorSetBindingValue> bindings) {
-  state.addOperands({device, setLayout});
-  SmallVector<Value, 4> bindingOrdinals;
-  SmallVector<Value, 4> bindingBuffers;
-  SmallVector<Value, 4> bindingOffsets;
-  SmallVector<Value, 4> bindingLengths;
-  for (auto binding : bindings) {
-    bindingOrdinals.push_back(binding.ordinal);
-    bindingBuffers.push_back(binding.buffer);
-    bindingOffsets.push_back(binding.byteOffset);
-    bindingLengths.push_back(binding.byteLength);
-  }
-  state.addOperands(bindingOrdinals);
-  state.addOperands(bindingBuffers);
-  state.addOperands(bindingOffsets);
-  state.addOperands(bindingLengths);
-}
-
-void DescriptorSetCreateOp::getAsmResultNames(
-    function_ref<void(Value, StringRef)> setNameFn) {
-  setNameFn(getResult(), "descriptor_set");
-}
-
 //===----------------------------------------------------------------------===//
 // hal.descriptor_set_layout.create
 //===----------------------------------------------------------------------===//

diff --git a/compiler/src/iree/compiler/Dialect/HAL/IR/HALOps.td b/compiler/src/iree/compiler/Dialect/HAL/IR/HALOps.td
@@ -907,32 +907,6 @@ def HAL_CommandBufferPushDescriptorSetOp :
   let hasCanonicalizer = 1;
 }
 
-def HAL_CommandBufferBindDescriptorSetOp :
-    HAL_Op<"command_buffer.bind_descriptor_set"> {
-  let summary = [{command buffer descriptor set binding operation}];
-  let description = [{
-    Binds a descriptor set to the given set number. The provided descriptor set
-    must not be modified once bound to a command buffer.
-  }];
-
-  let arguments = (ins
-    HAL_CommandBuffer:$command_buffer,
-    HAL_ExecutableLayout:$executable_layout,
-    Index:$set,
-    HAL_DescriptorSet:$descriptor_set,
-    Variadic<HAL_DeviceSize>:$dynamic_offsets
-  );
-
-  let assemblyFormat = [{
-    `<` $command_buffer `:` type($command_buffer) `>`
-    `layout` `(` $executable_layout `:` type($executable_layout) `)`
-    `` `[` $set `]`
-    `set` `(` $descriptor_set `:` type($descriptor_set) `)`
-    (`offsets` `(` `[` $dynamic_offsets^ `]` `)`)?
-    attr-dict-with-keyword
-  }];
-}
-
 def HAL_CommandBufferDispatchSymbolOp : HAL_Op<"command_buffer.dispatch.symbol"> {
   let summary = [{command buffer dispatch recording operation, using symbolref}];
   let description = [{
@@ -1070,53 +1044,6 @@ def HAL_ConstantStorageOp : HAL_Op<"constant_storage", [
   ];
 }
 
-//===----------------------------------------------------------------------===//
-// !hal.descriptor_set / iree_hal_descriptor_set_layout_t
-//===----------------------------------------------------------------------===//
-
-def HAL_DescriptorSetCreateOp : HAL_PureOp<"descriptor_set.create", [
-    DeclareOpInterfaceMethods<OpAsmOpInterface, ["getAsmResultNames"]>,
-    SameVariadicOperandSize,
-  ]> {
-  let summary = [{allocates a descriptor set from the device pool}];
-  let description = [{
-    Creates a DescriptorSet from the device pool.
-  }];
-
-  let arguments = (ins
-    HAL_Device:$device,
-    HAL_DescriptorSetLayout:$set_layout,
-    Variadic<Index>:$binding_ordinals,
-    Variadic<HAL_BufferType>:$binding_buffers,
-    Variadic<HAL_DeviceSize>:$binding_offsets,
-    Variadic<HAL_DeviceSize>:$binding_lengths
-  );
-  let results = (outs
-    HAL_DescriptorSet:$result
-  );
-
-  let assemblyFormat = [{
-    `device` `(` $device `:` type($device) `)`
-    `layout` `(` $set_layout `:` type($set_layout) `)`
-    `bindings` `(` `[`
-    custom<DescriptorSetBindings>($binding_ordinals,
-                                  $binding_buffers,
-                                  type($binding_buffers),
-                                  $binding_offsets,
-                                  $binding_lengths)
-    `]` `)`
-    attr-dict-with-keyword
-  }];
-
-  let builders = [
-    OpBuilder<(ins
-      "Value":$device,
-      "Value":$setLayout,
-      "ArrayRef<DescriptorSetBindingValue>":$bindings
-    )>,
-  ];
-}
-
 //===----------------------------------------------------------------------===//
 // !hal.descriptor_set_layout / iree_hal_descriptor_set_layout_t
 //===----------------------------------------------------------------------===//

diff --git a/compiler/src/iree/compiler/Dialect/HAL/IR/HALTypes.cpp b/compiler/src/iree/compiler/Dialect/HAL/IR/HALTypes.cpp
@@ -540,9 +540,8 @@ void HALDialect::registerAttributes() {
 
 void HALDialect::registerTypes() {
   addTypes<AllocatorType, BufferType, BufferViewType, CommandBufferType,
-           DescriptorSetType, DescriptorSetLayoutType, DeviceType, EventType,
-           ExecutableType, ExecutableLayoutType, FenceType, RingBufferType,
-           SemaphoreType>();
+           DescriptorSetLayoutType, DeviceType, EventType, ExecutableType,
+           ExecutableLayoutType, FenceType, RingBufferType, SemaphoreType>();
 }
 
 //===----------------------------------------------------------------------===//
@@ -582,7 +581,6 @@ Type HALDialect::parseType(DialectAsmParser &parser) const {
           .Case("buffer", BufferType::get(getContext()))
           .Case("buffer_view", BufferViewType::get(getContext()))
           .Case("command_buffer", CommandBufferType::get(getContext()))
-          .Case("descriptor_set", DescriptorSetType::get(getContext()))
           .Case("descriptor_set_layout",
                 DescriptorSetLayoutType::get(getContext()))
           .Case("device", DeviceType::get(getContext()))
@@ -609,8 +607,6 @@ void HALDialect::printType(Type type, DialectAsmPrinter &p) const {
     p << "buffer_view";
   } else if (type.isa<CommandBufferType>()) {
     p << "command_buffer";
-  } else if (type.isa<DescriptorSetType>()) {
-    p << "descriptor_set";
   } else if (type.isa<DescriptorSetLayoutType>()) {
     p << "descriptor_set_layout";
   } else if (type.isa<DeviceType>()) {

diff --git a/compiler/src/iree/compiler/Dialect/HAL/IR/HALTypes.h b/compiler/src/iree/compiler/Dialect/HAL/IR/HALTypes.h
@@ -87,11 +87,6 @@ struct CommandBufferType
   using Base::Base;
 };
 
-struct DescriptorSetType
-    : public Type::TypeBase<DescriptorSetType, Type, TypeStorage> {
-  using Base::Base;
-};
-
 struct DescriptorSetLayoutType
     : public Type::TypeBase<DescriptorSetLayoutType, Type, TypeStorage> {
   using Base::Base;

diff --git a/compiler/src/iree/compiler/Dialect/HAL/IR/test/command_buffer_ops.mlir b/compiler/src/iree/compiler/Dialect/HAL/IR/test/command_buffer_ops.mlir
@@ -99,38 +99,6 @@ func.func @command_buffer_copy_buffer(
 
 // -----
 
-// CHECK-LABEL: @command_buffer_bind_descriptor_set
-//  CHECK-SAME: (%[[CMD:.+]]: !hal.command_buffer,
-//  CHECK-SAME: %[[LAYOUT:.+]]: !hal.executable_layout,
-//  CHECK-SAME: %[[SET:.+]]: !hal.descriptor_set,
-//  CHECK-SAME: %[[OFFSET:.+]]: index)
-func.func @command_buffer_bind_descriptor_set(
-    %cmd: !hal.command_buffer,
-    %layout: !hal.executable_layout,
-    %set: !hal.descriptor_set,
-    %offset: index
-  ) {
-  // CHECK: %[[SET_IDX:.+]] = arith.constant 0
-  %c0 = arith.constant 0 : index
-  //      CHECK: hal.command_buffer.bind_descriptor_set<%[[CMD]] : !hal.command_buffer>
-  // CHECK-SAME:   layout(%[[LAYOUT]] : !hal.executable_layout)[%[[SET_IDX]]]
-  // CHECK-SAME:   set(%[[SET]] : !hal.descriptor_set)
-  hal.command_buffer.bind_descriptor_set<%cmd : !hal.command_buffer>
-      layout(%layout : !hal.executable_layout)[%c0]
-      set(%set : !hal.descriptor_set)
-  //      CHECK: hal.command_buffer.bind_descriptor_set<%[[CMD]] : !hal.command_buffer>
-  // CHECK-SAME:   layout(%[[LAYOUT]] : !hal.executable_layout)[%[[SET_IDX]]]
-  // CHECK-SAME:   set(%[[SET]] : !hal.descriptor_set)
-  // CHECK-SAME:   offsets([%[[OFFSET]]])
-  hal.command_buffer.bind_descriptor_set<%cmd : !hal.command_buffer>
-      layout(%layout : !hal.executable_layout)[%c0]
-      set(%set : !hal.descriptor_set)
-      offsets([%offset])
-  return
-}
-
-// -----
-
 hal.executable @ex {
   hal.executable.variant @backend, target = <"backend", "format"> {
     hal.executable.export @entry0 ordinal(0) layout(#hal.executable.layout<push_constants = 0, sets = [

diff --git a/compiler/src/iree/compiler/Dialect/HAL/Transforms/ElideRedundantCommands.cpp b/compiler/src/iree/compiler/Dialect/HAL/Transforms/ElideRedundantCommands.cpp
@@ -202,16 +202,6 @@ static LogicalResult processOp(IREE::HAL::CommandBufferPushDescriptorSetOp op,
   return success();
 }
 
-static LogicalResult processOp(IREE::HAL::CommandBufferBindDescriptorSetOp op,
-                               CommandBufferState &state) {
-  // TODO(benvanik): descriptor set binding.
-  // For now we just nuke the state.
-  auto *setState = state.getDescriptorSet(op.getSet());
-  if (!setState) return failure();
-  setState->clear();
-  return success();
-}
-
 class ElideRedundantCommandsPass
     : public PassWrapper<ElideRedundantCommandsPass, OperationPass<void>> {
  public:
@@ -270,12 +260,6 @@ class ElideRedundantCommandsPass
                   invalidateState(op.getCommandBuffer());
                 }
               })
-              .Case([&](IREE::HAL::CommandBufferBindDescriptorSetOp op) {
-                resetCommandBufferBarrierBit(op);
-                if (failed(processOp(op, stateMap[op.getCommandBuffer()]))) {
-                  invalidateState(op.getCommandBuffer());
-                }
-              })
               .Case<IREE::HAL::CommandBufferDeviceOp,
                     IREE::HAL::CommandBufferBeginDebugGroupOp,
                     IREE::HAL::CommandBufferEndDebugGroupOp,

diff --git a/compiler/src/iree/compiler/Dialect/HAL/hal.imports.mlir b/compiler/src/iree/compiler/Dialect/HAL/hal.imports.mlir
@@ -235,15 +235,6 @@ vm.import @command_buffer.push_descriptor_set(
   %bindings : tuple<i32, !vm.ref<!hal.buffer>, i64, i64>...
 )
 
-// Binds a descriptor set to the given set number.
-vm.import @command_buffer.bind_descriptor_set(
-  %command_buffer : !vm.ref<!hal.command_buffer>,
-  %executable_layout : !vm.ref<!hal.executable_layout>,
-  %set : i32,
-  %descriptor_set : !vm.ref<!hal.descriptor_set>,
-  %dynamic_offsets : i64 ...
-)
-
 // Dispatches an execution request.
 vm.import @command_buffer.dispatch(
   %command_buffer : !vm.ref<!hal.command_buffer>,
@@ -264,18 +255,6 @@ vm.import @command_buffer.dispatch.indirect(
   %workgroups_offset : i64
 )
 
-//===----------------------------------------------------------------------===//
-// iree_hal_descriptor_set_t
-//===----------------------------------------------------------------------===//
-
-// Creates a new immutable descriptor set based on the given layout.
-vm.import @descriptor_set.create(
-  %device : !vm.ref<!hal.device>,
-  %set_layout : !vm.ref<!hal.descriptor_set_layout>,
-  // <binding, buffer, offset, length>
-  %bindings : tuple<i32, !vm.ref<!hal.buffer>, i64, i64>...
-) -> !vm.ref<!hal.descriptor_set>
-
 //===----------------------------------------------------------------------===//
 // iree_hal_descriptor_set_layout_t
 //===----------------------------------------------------------------------===//

diff --git a/experimental/rocm/cts/CMakeLists.txt b/experimental/rocm/cts/CMakeLists.txt
@@ -21,8 +21,6 @@ iree_hal_cts_test_suite(
     # This test depends on iree_hal_rocm_direct_command_buffer_update_buffer
     # via iree_hal_buffer_view_allocate_buffer, which is not implemented yet.
     "command_buffer_dispatch"
-    # Non-push descriptor sets are not implemented in the ROCm backend yet.
-    "descriptor_set"
     # Semaphores are not implemented in the ROCm backend yet.
     "semaphore_submission"
     "semaphore"

diff --git a/experimental/rocm/direct_command_buffer.c b/experimental/rocm/direct_command_buffer.c
@@ -319,16 +319,6 @@ static iree_status_t iree_hal_rocm_direct_command_buffer_push_descriptor_set(
   return iree_ok_status();
 }
 
-static iree_status_t iree_hal_rocm_direct_command_buffer_bind_descriptor_set(
-    iree_hal_command_buffer_t* base_command_buffer,
-    iree_hal_executable_layout_t* executable_layout, uint32_t set,
-    iree_hal_descriptor_set_t* descriptor_set,
-    iree_host_size_t dynamic_offset_count,
-    const iree_device_size_t* dynamic_offsets) {
-  return iree_make_status(IREE_STATUS_UNIMPLEMENTED,
-                          "need rocm implementation");
-}
-
 static iree_status_t iree_hal_rocm_direct_command_buffer_dispatch(
     iree_hal_command_buffer_t* base_command_buffer,
     iree_hal_executable_t* executable, int32_t entry_point,
@@ -394,8 +384,6 @@ static const iree_hal_command_buffer_vtable_t
         .push_constants = iree_hal_rocm_direct_command_buffer_push_constants,
         .push_descriptor_set =
             iree_hal_rocm_direct_command_buffer_push_descriptor_set,
-        .bind_descriptor_set =
-            iree_hal_rocm_direct_command_buffer_bind_descriptor_set,
         .dispatch = iree_hal_rocm_direct_command_buffer_dispatch,
         .dispatch_indirect =
             iree_hal_rocm_direct_command_buffer_dispatch_indirect,

diff --git a/experimental/rocm/rocm_device.c b/experimental/rocm/rocm_device.c
@@ -192,16 +192,6 @@ static iree_status_t iree_hal_rocm_device_create_command_buffer(
       queue_affinity, &device->block_pool, out_command_buffer);
 }
 
-static iree_status_t iree_hal_rocm_device_create_descriptor_set(
-    iree_hal_device_t* base_device,
-    iree_hal_descriptor_set_layout_t* set_layout,
-    iree_host_size_t binding_count,
-    const iree_hal_descriptor_set_binding_t* bindings,
-    iree_hal_descriptor_set_t** out_descriptor_set) {
-  return iree_make_status(IREE_STATUS_UNIMPLEMENTED,
-                          "non-push descriptor sets still need work");
-}
-
 static iree_status_t iree_hal_rocm_device_create_descriptor_set_layout(
     iree_hal_device_t* base_device,
     iree_hal_descriptor_set_layout_usage_type_t usage_type,
@@ -320,7 +310,6 @@ static const iree_hal_device_vtable_t iree_hal_rocm_device_vtable = {
     .trim = iree_hal_rocm_device_trim,
     .query_i64 = iree_hal_rocm_device_query_i64,
     .create_command_buffer = iree_hal_rocm_device_create_command_buffer,
-    .create_descriptor_set = iree_hal_rocm_device_create_descriptor_set,
     .create_descriptor_set_layout =
         iree_hal_rocm_device_create_descriptor_set_layout,
     .create_event = iree_hal_rocm_device_create_event,