Skip to content
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.

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

Attaching pipeline layout to hal.interface.binding.subspan & co. #18098

Merged
merged 4 commits into from
Aug 5, 2024

Conversation

benvanik
Copy link
Collaborator

@benvanik benvanik commented Aug 2, 2024

This allows for the whole layout to be known locally when lowering out of HAL and into target-specific binding data structures. This information was (and is still) available on the exports but annoying to get to and not present in all tests. This allowed removing the descriptor type from the subspan op and will allow for us to have non-i32 push constant types in the future. Verifiers were added for both push constant and descriptor set/binding ordinals now that the information is cheap to verify.

Progress on #17875 (this is needed for lowering non-0 ordinal descriptor sets to CPU/CUDA/ROCM targets).

@benvanik benvanik added the compiler/dialects Relating to the IREE compiler dialects (flow, hal, vm) label Aug 2, 2024
@benvanik benvanik requested a review from ScottTodd August 2, 2024 22:12
This allows for the whole layout to be known locally when lowering out
of HAL and into target-specific binding data structures. This information
was (and is still) available on the exports but annoying to get to and
not present in all tests.
@benvanik benvanik force-pushed the users/benvanik/binding-layout branch from 28e7acf to bdb22a5 Compare August 2, 2024 22:26
@benvanik
Copy link
Collaborator Author

benvanik commented Aug 2, 2024

I split the codegen changes out from the actual dialect changes. The current failures are WGSL-related due to push constant emulation adding a descriptor set without updating the pipeline layout - I'll see if I can fix that. WGSL fixed!

This isn't fantastic as it doesn't update other attrs in the program
but it was always doing magic like this so it's probably ok until we
want to rework WebGPU support.
@benvanik benvanik force-pushed the users/benvanik/binding-layout branch from b78e194 to 06c6642 Compare August 4, 2024 00:57
Comment on lines -17 to +20
%input_0 = hal.interface.constant.load[0] : i32
%input_1 = hal.interface.constant.load[1] : i32
%input_2 = hal.interface.constant.load[2] : i32
%input_3 = hal.interface.constant.load[3] : i32
%input_0 = hal.interface.constant.load layout(#pipeline_layout) ordinal(0) : i32
%input_1 = hal.interface.constant.load layout(#pipeline_layout) ordinal(1) : i32
%input_2 = hal.interface.constant.load layout(#pipeline_layout) ordinal(2) : i32
%input_3 = hal.interface.constant.load layout(#pipeline_layout) ordinal(3) : i32
Copy link
Member

Choose a reason for hiding this comment

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

Nice having these explicit. Needing to look them up by walking the IR to find an export op was annoying :P

Can we ever end up using multiple pipeline layouts at this point? Maybe we could have a verifier that enforces only one?

#pipeline_layout_0 = #hal.pipeline.layout<push_constants = 4, sets = [
  #hal.descriptor_set.layout<0, bindings = [
    #hal.descriptor_set.binding<0, storage_buffer>
  ]>
]>

#pipeline_layout_1 = #hal.pipeline.layout<push_constants = 2, sets = [
  #hal.descriptor_set.layout<0, bindings = [
    #hal.descriptor_set.binding<0, storage_buffer>
  ]>
]>

hal.executable.source public @executable {
  hal.executable.export public @write_push_constants ordinal(0) layout(#pipeline_layout) attributes {workgroup_size = [1 : index, 1 : index, 1 : index]} {
  ^bb0(%arg0: !hal.device):
    %c1 = arith.constant 1 : index
    hal.return %c1, %c1, %c1 : index, index, index
  }
  builtin.module {
    func.func @write_push_constants() {
      %input_0 = hal.interface.constant.load layout(#pipeline_layout_0) ordinal(0) : i32
      %input_1 = hal.interface.constant.load layout(#pipeline_layout_1) ordinal(0) : i32

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's possible so long as they are compatible - this could let us have multiple exported entry points that share some subset of buffers but have differing push constant counts, etc (so two top level functions that fetch push constants/buffers, then a common function that can still reference the base push constants/buffers). We could have a verifier on the executable that maybe poked in and looked for them, but it's likely going to need to be a dedicated pass given how much of the IR tree it pulls together. We'd also then have to codify what pipeline compatibility is and I'm not yet sure we know if we even want to allow that so I'm punting on it for now :P

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I figured the analysis would be complex enough that it would need to be a full pass and not a local verifier.

@benvanik benvanik merged commit 2193406 into main Aug 5, 2024
45 checks passed
@benvanik benvanik deleted the users/benvanik/binding-layout branch August 5, 2024 16:49
yzhang93 added a commit to nod-ai/iree-amd-aie that referenced this pull request Aug 9, 2024
Following runtime changes in IREE
- Attaching pipeline layout to hal.interface.binding.subspan.
iree-org/iree#18098
- Adding flag placeholders to semaphores/events.
iree-org/iree#18122
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/dialects Relating to the IREE compiler dialects (flow, hal, vm)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants