naga_oil icon indicating copy to clipboard operation
naga_oil copied to clipboard

Alignment attribute is not applied when defined in modules.

Open Swoorup opened this issue 2 years ago • 7 comments

This example is straight from the w3 examples which follows the valid path. It appears that the align(16) attribute is not applied when the struct is defined in a composable module rather than naga module.

EDIT: I am not sure this issue also extends to @size attribute.

 composer
     .add_composable_module(ComposableModuleDescriptor {
        source: r#"
          struct S {
            x: f32
          }
        
          struct Valid {
            a: S,
           @align(16) b: f32 // valid: offset between a and b is 16 bytes
          }
        "#,
        file_path: "valid.wgsl",
        as_name: Some("valid".to_owned()),
        ..Default::default()
    })
    .unwrap();

composer
    .make_naga_module(NagaModuleDescriptor {
        source: r#"
            #import valid::Valid;
            @group(0) @binding(1) var<uniform> valid: Valid;
        "#,
        file_path: "",
        ..Default::default()
      })
      .map_err(|err| println!("{}", err.emit_to_string(&composer)))
      .unwrap();

The above fails with

error: failed to build a valid final module: Global variable [1] 'valid' is invalid
  ┌─ :3:33
  │
3 │           @group(0) @binding(1) var<uniform> valid: valid::Valid;
  │                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ naga::GlobalVariable [1]
  │
  = Alignment requirements for address space Uniform are not met by [4]
  = The struct member[1] offset 4 must be at least 16

This works however when everything is defined in a single file.

Swoorup avatar Feb 25 '24 16:02 Swoorup

The success case for brevity:

composer
    .make_naga_module(NagaModuleDescriptor {
        source: r#"
      struct S {
        x: f32
      }
      
      struct Valid {
        a: S,
        @align(16) b: f32 // valid: offset between a and b is 16 bytes
      }
      @group(0) @binding(1) var<uniform> valid: Valid;
    "#,
        file_path: "",
        ..Default::default()
    })
    .unwrap();

Swoorup avatar Feb 25 '24 16:02 Swoorup

Because of this issue or something else I also run into other weird issue when used inside function from different file as well. It is perhaps treating the type separately due to the type contents as well.

 struct S {
  x: f32
}

struct Valid {
  a: S,
  @align(16) b: f32 // valid: offset between a and b is 16 bytes
}

fn weird_crap(v: Valid) {}
#import valid::{Valid, weird_crap, S};

fn test() {
  let s = S(1.0);
  let v = Valid(s, 2.9);
  weird_crap(v);
}
error: failed to build a valid final module: Function [2] 'test' is invalid
  ┌─ :4:11
  │  
4 │ ╭           fn test() {
5 │ │             let s = valid::S(1.0);
6 │ │             let v = valid::Valid(s, 2.9);
  │ │                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ naga::Expression [4]
7 │ │             valid::weird_crap(v);
  │ │             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ invalid function call
  │ ╰───────────────────────────────────────────────────^ naga::Function [2]
  │  
  = Call to [1] is invalid
  = Argument 0 value [4] doesn't match the type [3]


thread 'main' panicked at examples/pbr_compose_test.rs:316:10:

Swoorup avatar Feb 26 '24 00:02 Swoorup

Probably related: https://github.com/gfx-rs/wgpu/issues/4377

Swoorup avatar Feb 26 '24 02:02 Swoorup

There are naga issues related to this (wgsl and glsl).

In bevy we still use dummy padding items (_padding: u32) instead of the align attribute to work around it.

I’ll investigate again though because this doesn’t seem exactly related to the output issues, it’s a pure naga issue.

robtfm avatar Feb 26 '24 07:02 robtfm

on reflection, the backend issues are the direct cause - the including module is built from a generated header for the include for the target language (which includes the struct definition), and that header is generated incorrectly due to the backend issues.

robtfm avatar Feb 26 '24 09:02 robtfm

on reflection, the backend issues are the direct cause - the including module is built from a generated header for the include for the target language (which includes the struct definition), and that header is generated incorrectly due to the backend issues.

afaik from my very rough understanding, naga oil works with the backend or the lower end of the wgsl from naga rather than the wgsl frontend. Doesn't it make more sense to work with naga::front::wgsl::parse::ast::TranslationUnit rather than naga::Module as it preserves some, if not all of the source information?

I assume information about alias issue is also lost at the backend?

Swoorup avatar Feb 26 '24 09:02 Swoorup

naga_oil currently supports modules/shaders written in a mix of wgsl and glsl, and (if anybody wanted to add it) could support other naga-supported langauges as well. so i would much prefer fixing the naga issues directly rather than working around them and limiting the scope - from my brief look it doesn't seem like anything fundamentally hard to fix in naga.

robtfm avatar Feb 26 '24 09:02 robtfm