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.

Sign up for GitHub

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

Jump to bottom

spirv-opt: copy-propagate-arrays is generating invalid OpAccessChain instructions #4887

Closed
Keenuts opened this issue Aug 9, 2022 · 1 comment
Closed

spirv-opt: copy-propagate-arrays is generating invalid OpAccessChain instructions #4887

Keenuts opened this issue Aug 9, 2022 · 1 comment
Assignees
@Keenuts
Labels
component:optimization

Comments

@Keenuts
Copy link
Contributor

Keenuts commented Aug 9, 2022

This is the cause of microsoft/DirectXShaderCompiler#4429.

The optimization pass tries to optimize loads by removing the superfluous ones. But the optimization doesn't check if we index in a struct or not, and can generate bad code in some cases:

cbuffer TestPositions {
    uint my_input;
};

uint main(uint my_index : SV_VertexID) : SV_ShadingRate {
    const uint my_array[1] = { my_input };
    const uint my_tmp = my_array[my_index];
    return my_tmp;
}
@Keenuts Keenuts self-assigned this Aug 9, 2022
Keenuts added a commit to Keenuts/SPIRV-Tools that referenced this issue Aug 9, 2022
@Keenuts
fix copy-propagate-arrays index opti on structs.
43b2646 
As per SPIR-V spec:
OpAccessChain indices must be OpConstant when indexing into a structure.

This optimization tried to remove load cascade. But in some scenario
failed:

```c
cbuffer MyStruct {
    uint my_field;
};

uint main(uint index) {
    const uint my_array[1] = { my_field };
    return my_array[index]
}
```

This is valid as the struct is indexed with a constant index, and then
the array is indexed using a dynamic index.
The optimization would consider the local array to be useless and
generated a load directly into the struct.

Fixes KhronosGroup#4887

Signed-off-by: Nathan Gauër <brioche@google.com>
@Keenuts
Copy link
Contributor Author

Keenuts commented Aug 9, 2022

This commit seems to fix this specif issue, but now we generate an additional constant in the code.
This is fine in the end because we clean it with a DCE pass, but we shouldn't.

43b2646

Working on it.

Keenuts added a commit to Keenuts/SPIRV-Tools that referenced this issue Aug 9, 2022
@Keenuts
spirv-opt: cleanup instructions for cancelled CompositeExtract optimi…
09aa8b6 
…zations

When an OpAccessChain is using temporaries to store indices extracted
using OpCompositeExtract, the optimizer will try bypass it by rewritting
the OpAccessChain to directly fetch from the extract.
This is done by creating new indices.

Until now, new constants were created, even if the OpAccessChain was not
patched (optimization canceled). This was considered OK since a DCE pass
would probably run after. This commit adds a way to clean up these unused
instruction in case the optimization is dropped.

Fixes KhronosGroup#4887

Signed-off-by: Nathan Gauër <brioche@google.com>
Keenuts added a commit to Keenuts/SPIRV-Tools that referenced this issue Aug 9, 2022
@Keenuts
spirv-opt: fix copy-propagate-arrays index opti on structs.
5405223 
As per SPIR-V spec:
OpAccessChain indices must be OpConstant when indexing into a structure.

This optimization tried to remove load cascade. But in some scenario
failed:

```c
cbuffer MyStruct {
    uint my_field;
};

uint main(uint index) {
    const uint my_array[1] = { my_field };
    return my_array[index]
}
```

This is valid as the struct is indexed with a constant index, and then
the array is indexed using a dynamic index.
The optimization would consider the local array to be useless and
generated a load directly into the struct.

Fixes KhronosGroup#4887

Signed-off-by: Nathan Gauër <brioche@google.com>
@Keenuts Keenuts mentioned this issue Aug 11, 2022
Merged
Keenuts added a commit to Keenuts/SPIRV-Tools that referenced this issue Aug 11, 2022
@Keenuts
spirv-opt: fix copy-propagate-arrays index opti on structs.
ceb6091 
As per SPIR-V spec:
OpAccessChain indices must be OpConstant when indexing into a structure.

This optimization tried to remove load cascade. But in some scenario
failed:

```c
cbuffer MyStruct {
    uint my_field;
};

uint main(uint index) {
    const uint my_array[1] = { my_field };
    return my_array[index]
}
```

This is valid as the struct is indexed with a constant index, and then
the array is indexed using a dynamic index.
The optimization would consider the local array to be useless and
generated a load directly into the struct.

Fixes KhronosGroup#4887

Signed-off-by: Nathan Gauër <brioche@google.com>
Keenuts added a commit to Keenuts/SPIRV-Tools that referenced this issue Aug 11, 2022
@Keenuts
spirv-opt: fix copy-propagate-arrays index opti on structs.
907d042 
As per SPIR-V spec:
OpAccessChain indices must be OpConstant when indexing into a structure.

This optimization tried to remove load cascade. But in some scenario
failed:

```c
cbuffer MyStruct {
    uint my_field;
};

uint main(uint index) {
    const uint my_array[1] = { my_field };
    return my_array[index]
}
```

This is valid as the struct is indexed with a constant index, and then
the array is indexed using a dynamic index.
The optimization would consider the local array to be useless and
generated a load directly into the struct.

Fixes KhronosGroup#4887

Signed-off-by: Nathan Gauër <brioche@google.com>
Keenuts added a commit to Keenuts/SPIRV-Tools that referenced this issue Aug 11, 2022
@Keenuts
spirv-opt: fix copy-propagate-arrays index opti on structs.
1d91d65 
As per SPIR-V spec:
OpAccessChain indices must be OpConstant when indexing into a structure.

This optimization tried to remove load cascade. But in some scenario
failed:

```c
cbuffer MyStruct {
    uint my_field;
};

uint main(uint index) {
    const uint my_array[1] = { my_field };
    return my_array[index]
}
```

This is valid as the struct is indexed with a constant index, and then
the array is indexed using a dynamic index.
The optimization would consider the local array to be useless and
generated a load directly into the struct.

Fixes KhronosGroup#4887

Signed-off-by: Nathan Gauër <brioche@google.com>
Keenuts added a commit to Keenuts/SPIRV-Tools that referenced this issue Aug 12, 2022
@Keenuts
spirv-opt: fix copy-propagate-arrays index opti on structs.
8610198 
As per SPIR-V spec:
OpAccessChain indices must be OpConstant when indexing into a structure.

This optimization tried to remove load cascade. But in some scenario
failed:

```c
cbuffer MyStruct {
    uint my_field;
};

uint main(uint index) {
    const uint my_array[1] = { my_field };
    return my_array[index]
}
```

This is valid as the struct is indexed with a constant index, and then
the array is indexed using a dynamic index.
The optimization would consider the local array to be useless and
generated a load directly into the struct.

Fixes KhronosGroup#4887

Signed-off-by: Nathan Gauër <brioche@google.com>
Keenuts added a commit to Keenuts/SPIRV-Tools that referenced this issue Aug 15, 2022
@Keenuts
spirv-opt: fix copy-propagate-arrays index opti on structs.
b783872 
As per SPIR-V spec:
OpAccessChain indices must be OpConstant when indexing into a structure.

This optimization tried to remove load cascade. But in some scenario
failed:

```c
cbuffer MyStruct {
    uint my_field;
};

uint main(uint index) {
    const uint my_array[1] = { my_field };
    return my_array[index]
}
```

This is valid as the struct is indexed with a constant index, and then
the array is indexed using a dynamic index.
The optimization would consider the local array to be useless and
generated a load directly into the struct.

Fixes KhronosGroup#4887

Signed-off-by: Nathan Gauër <brioche@google.com>
Keenuts added a commit to Keenuts/SPIRV-Tools that referenced this issue Aug 15, 2022
@Keenuts
spirv-opt: fix copy-propagate-arrays index opti on structs.
925a6a5 
As per SPIR-V spec:
OpAccessChain indices must be OpConstant when indexing into a structure.

This optimization tried to remove load cascade. But in some scenario
failed:

```c
cbuffer MyStruct {
    uint my_field;
};

uint main(uint index) {
    const uint my_array[1] = { my_field };
    return my_array[index]
}
```

This is valid as the struct is indexed with a constant index, and then
the array is indexed using a dynamic index.
The optimization would consider the local array to be useless and
generated a load directly into the struct.

Fixes KhronosGroup#4887

Signed-off-by: Nathan Gauër <brioche@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Keenuts Keenuts

Labels
component:optimization
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@Keenuts