Skip to content

PIX: Account for alignment when computing member offset#4903

Merged
jeffnn merged 2 commits intomicrosoft:mainfrom
jeffnn:PIX_Embedded2DArray
Jan 10, 2023
Merged

PIX: Account for alignment when computing member offset#4903
jeffnn merged 2 commits intomicrosoft:mainfrom
jeffnn:PIX_Embedded2DArray

Conversation

@jeffnn
Copy link
Copy Markdown
Collaborator

@jeffnn jeffnn commented Dec 23, 2022

This has been a long-standing issue for PIX. Certain structs (with e.g., embedded arrays of structs) don't pay attention to their alignment requirements.

@AppVeyorBot
Copy link
Copy Markdown

@Keenuts
Copy link
Copy Markdown
Collaborator

Keenuts commented Jan 4, 2023

Hi! Thanks for the PR. Don't own the DXIL part, so cannot approve, but tried it, and I see no difference between in the generated DXIL for this shader taken from the test between this PR and HEAD (a0b6fc1), is that expected?

struct EmbeddedStruct
{
    uint32_t TwoDArray[2][2];
};

struct smallPayload
{
    uint32_t OneInt;
    EmbeddedStruct embeddedStruct;
    uint64_t bigOne;
};

[numthreads(1, 1, 1)]
void main()
{
    smallPayload p;
    p.OneInt = -137;
    p.embeddedStruct.TwoDArray[0][0] = 252;
    p.embeddedStruct.TwoDArray[0][1] = 253;
    p.embeddedStruct.TwoDArray[1][0] = 254;
    p.embeddedStruct.TwoDArray[1][1] = 255;
    p.bigOne = 123456789;

    DispatchMesh(2, 1, 1, p);
}

cmd: ./build/bin/dxc ./repro.hlsl -T as_6_5 -E main -Zi

@jeffnn
Copy link
Copy Markdown
Collaborator Author

jeffnn commented Jan 9, 2023

Hi! Thanks for the PR. Don't own the DXIL part, so cannot approve, but tried it, and I see no difference between in the generated DXIL for this shader taken from the test between this PR and HEAD (a0b6fc1), is that expected?

struct EmbeddedStruct
{
    uint32_t TwoDArray[2][2];
};

struct smallPayload
{
    uint32_t OneInt;
    EmbeddedStruct embeddedStruct;
    uint64_t bigOne;
};

[numthreads(1, 1, 1)]
void main()
{
    smallPayload p;
    p.OneInt = -137;
    p.embeddedStruct.TwoDArray[0][0] = 252;
    p.embeddedStruct.TwoDArray[0][1] = 253;
    p.embeddedStruct.TwoDArray[1][0] = 254;
    p.embeddedStruct.TwoDArray[1][1] = 255;
    p.bigOne = 123456789;

    DispatchMesh(2, 1, 1, p);
}

cmd: ./build/bin/dxc ./repro.hlsl -T as_6_5 -E main -Zi

Hi. This isn't expected to produce any difference in generated code. It's related to how debug information is processed, specifically to how that debug information reflects placement (according to the various structure packing rules) for embedded arrays like in the example.

Comment thread lib/DxilDia/DxilDiaSymbolManager.cpp Outdated
Copy link
Copy Markdown
Contributor

@adam-yang adam-yang left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@AppVeyorBot
Copy link
Copy Markdown

@jeffnn jeffnn merged commit 32d3deb into microsoft:main Jan 10, 2023
@jeffnn jeffnn deleted the PIX_Embedded2DArray branch January 10, 2023 01:24
jeffnn added a commit to jeffnn/DirectXShaderCompiler that referenced this pull request Jan 13, 2023
* Account for alignment

* Use llvm::RoundUpToAlignment
jeffnn added a commit to jeffnn/DirectXShaderCompiler that referenced this pull request Jan 13, 2023
* Account for alignment

* Use llvm::RoundUpToAlignment
jeffnn added a commit to jeffnn/DirectXShaderCompiler that referenced this pull request Jan 13, 2023
* Account for alignment

* Use llvm::RoundUpToAlignment
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

Successfully merging this pull request may close these issues.

4 participants