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

Alignment attribute is not applied when defined in modules. #78

Open
Swoorup opened this issue Feb 25, 2024 · 7 comments
Open

Alignment attribute is not applied when defined in modules. #78

Swoorup opened this issue Feb 25, 2024 · 7 comments

Comments

@Swoorup
Copy link

Swoorup commented Feb 25, 2024

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
Copy link
Author

Swoorup commented Feb 25, 2024

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
Copy link
Author

Swoorup commented Feb 26, 2024

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
Copy link
Author

Swoorup commented Feb 26, 2024

Probably related: gfx-rs/wgpu#4377

@robtfm
Copy link
Collaborator

robtfm commented Feb 26, 2024

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
Copy link
Collaborator

robtfm commented Feb 26, 2024

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.

@Swoorup
Copy link
Author

Swoorup commented Feb 26, 2024

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?

@robtfm
Copy link
Collaborator

robtfm commented Feb 26, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants