[mlir][spirv] Fix SSA Handling in SPIRV -> MLIR Translation #123371
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
mishaobu
commented
Jan 17, 2025
| // uses come from blocks that won't be sinked into the SelectionOp/LoopOp's | ||
| // region. We cannot handle such cases given that once a value is sinked into | ||
| // the SelectionOp/LoopOp's region, there is no escape for it: | ||
| // SelectionOp/LooOp does not support yield values right now. |
mishaobu
changed the title
Contributor
|
Hey @mishaobu, Are you trying to solve a problem that arises when some variables are sank into a selection region, but are also used after the selection region, and the structurization fails? |
Contributor
Author
|
@IgWod-IMG Yes |
Contributor
|
I also faced the same problem and developed an alternative fix for it. I haven't submitted it yet, but you can see it here: https://github.com/imaginationtech/llvm-project/tree/img_split-cond-bb The idea is to basically split basic blocks that end with a conditional branch, so the rest of the block is not being sank. I've been still thinking what's the best and correct approach, so I wasn't sharing it yet, but since you opened your PR I thought I may share my solution. If you think that's something worth discussing, we could create a Discourse thread on that. |
Contributor
Author
|
@IgWod-IMG Thank you for sharing, definitely worth discussing but on my end I've had to step away from this for the time being. To be honest, building MLIR locally with the changes in this PR fulfills my needs. I have been wondering what is the proper way to write tests for these changes though -- it appears to me that an MLIR based test can never actually represent the valid SPVASM -> invalid MLIR scenario that our changes seek to resolve. Note the test failure in this PR's automation is only due to the nonstandard test format // usage of |
Member
I think that the llvm spirv backend uses spirv tools conditionally -- we could do something similar in a very limited set of tests. See https://github.com/llvm/llvm-project/blob/main/llvm/test/Other/spirv-sim/simple.spv . |
IgWod-IMG
added a commit
to imaginationtech/llvm-project
that referenced
this pull request
Feb 18, 2025
With the current design some of the values are sank into a selection region,
despite them being also used outside that region. This is because the current
deserializer logic sinks the entire basic block containing a conditional branch
forming a header of a selection construct, without accounting for some values
being used outside. This manifests as (for example):
```
<unknown>:0: error: 'spirv.Variable' op failed control flow structurization: it has uses outside of the enclosing selection/loop construct
<unknown>:0: note: see current operation: %0 = "spirv.Variable"()<{storage_class = #spirv.storage_class<Function>}> : () -> !spirv.ptr<vector<4xf32>, Function>
```
The proposed solution to this problem is to split the conditional basic block
into two, one block containing just the conditional branch, and other the rest
of instructions. By doing this, the logic that structures selection regions,
only sinks the comparison, keeping the rest of instructions outside the
selection region.
A SPIR-V test is required, as the problem can happen only during
deserialization and cannot be tested with `--test-spirv-roundtrip`. An MLIR
test exhibiting the problematic behaviour would be an incorrect MLIR in the
first place.
This solution is proposed as an alternative to an unfinished PR llvm#123371, that
is unlikely to be merged in the foreseeable future, as the author "stepped away
from this for a time being". There is also a Discourse thread:
https://discourse.llvm.org/t/spir-v-uses-outside-the-selection-region/84494
that tried to solicit some feedback on the topic.
IgWod-IMG
added a commit
to imaginationtech/llvm-project
that referenced
this pull request
Feb 19, 2025
With the current design some of the values are sank into a selection region,
despite them being also used outside that region. This is because the current
deserializer logic sinks the entire basic block containing a conditional branch
forming a header of a selection construct, without accounting for some values
being used outside. This manifests as (for example):
```
<unknown>:0: error: 'spirv.Variable' op failed control flow structurization: it has uses outside of the enclosing selection/loop construct
<unknown>:0: note: see current operation: %0 = "spirv.Variable"()<{storage_class = #spirv.storage_class<Function>}> : () -> !spirv.ptr<vector<4xf32>, Function>
```
The proposed solution to this problem is to split the conditional basic block
into two, one block containing just the conditional branch, and other the rest
of instructions. By doing this, the logic that structures selection regions,
only sinks the comparison, keeping the rest of instructions outside the
selection region.
A SPIR-V test is required, as the problem can happen only during
deserialization and cannot be tested with `--test-spirv-roundtrip`. An MLIR
test exhibiting the problematic behaviour would be an incorrect MLIR in the
first place.
This solution is proposed as an alternative to an unfinished PR llvm#123371, that
is unlikely to be merged in the foreseeable future, as the author "stepped away
from this for a time being". There is also a Discourse thread:
https://discourse.llvm.org/t/spir-v-uses-outside-the-selection-region/84494
that tried to solicit some feedback on the topic.
IgWod-IMG
added a commit
to imaginationtech/llvm-project
that referenced
this pull request
Feb 19, 2025
With the current design some of the values are sank into a selection region,
despite them being also used outside that region. This is because the current
deserializer logic sinks the entire basic block containing a conditional branch
forming a header of a selection construct, without accounting for some values
being used outside. This manifests as (for example):
```
<unknown>:0: error: 'spirv.Variable' op failed control flow structurization: it has uses outside of the enclosing selection/loop construct
<unknown>:0: note: see current operation: %0 = "spirv.Variable"()<{storage_class = #spirv.storage_class<Function>}> : () -> !spirv.ptr<vector<4xf32>, Function>
```
The proposed solution to this problem is to split the conditional basic block
into two, one block containing just the conditional branch, and other the rest
of instructions. By doing this, the logic that structures selection regions,
only sinks the comparison, keeping the rest of instructions outside the
selection region.
A SPIR-V test is required, as the problem can happen only during
deserialization and cannot be tested with `--test-spirv-roundtrip`. An MLIR
test exhibiting the problematic behaviour would be an incorrect MLIR in the
first place.
This solution is proposed as an alternative to an unfinished PR llvm#123371, that
is unlikely to be merged in the foreseeable future, as the author "stepped away
from this for a time being". There is also a Discourse thread:
https://discourse.llvm.org/t/spir-v-uses-outside-the-selection-region/84494
that tried to solicit some feedback on the topic.
IgWod-IMG
added a commit
to imaginationtech/llvm-project
that referenced
this pull request
Feb 20, 2025
With the current design some of the values are sank into a selection region,
despite them being also used outside that region. This is because the current
deserializer logic sinks the entire basic block containing a conditional branch
forming a header of a selection construct, without accounting for some values
being used outside. This manifests as (for example):
```
<unknown>:0: error: 'spirv.Variable' op failed control flow structurization: it has uses outside of the enclosing selection/loop construct
<unknown>:0: note: see current operation: %0 = "spirv.Variable"()<{storage_class = #spirv.storage_class<Function>}> : () -> !spirv.ptr<vector<4xf32>, Function>
```
The proposed solution to this problem is to split the conditional basic block
into two, one block containing just the conditional branch, and other the rest
of instructions. By doing this, the logic that structures selection regions,
only sinks the comparison, keeping the rest of instructions outside the
selection region.
A SPIR-V test is required, as the problem can happen only during
deserialization and cannot be tested with `--test-spirv-roundtrip`. An MLIR
test exhibiting the problematic behaviour would be an incorrect MLIR in the
first place.
This solution is proposed as an alternative to an unfinished PR llvm#123371, that
is unlikely to be merged in the foreseeable future, as the author "stepped away
from this for a time being". There is also a Discourse thread:
https://discourse.llvm.org/t/spir-v-uses-outside-the-selection-region/84494
that tried to solicit some feedback on the topic.
IgWod-IMG
added a commit
to imaginationtech/llvm-project
that referenced
this pull request
Feb 24, 2025
With the current design some of the values are sank into a selection region,
despite them being also used outside that region. This is because the current
deserializer logic sinks the entire basic block containing a conditional branch
forming a header of a selection construct, without accounting for some values
being used outside. This manifests as (for example):
```
<unknown>:0: error: 'spirv.Variable' op failed control flow structurization: it has uses outside of the enclosing selection/loop construct
<unknown>:0: note: see current operation: %0 = "spirv.Variable"()<{storage_class = #spirv.storage_class<Function>}> : () -> !spirv.ptr<vector<4xf32>, Function>
```
The proposed solution to this problem is to split the conditional basic block
into two, one block containing just the conditional branch, and other the rest
of instructions. By doing this, the logic that structures selection regions,
only sinks the comparison, keeping the rest of instructions outside the
selection region.
A SPIR-V test is required, as the problem can happen only during
deserialization and cannot be tested with `--test-spirv-roundtrip`. An MLIR
test exhibiting the problematic behaviour would be an incorrect MLIR in the
first place.
This solution is proposed as an alternative to an unfinished PR llvm#123371, that
is unlikely to be merged in the foreseeable future, as the author "stepped away
from this for a time being". There is also a Discourse thread:
https://discourse.llvm.org/t/spir-v-uses-outside-the-selection-region/84494
that tried to solicit some feedback on the topic.
IgWod-IMG
added a commit
to imaginationtech/llvm-project
that referenced
this pull request
Feb 24, 2025
With the current design some of the values are sank into a selection region,
despite them being also used outside that region. This is because the current
deserializer logic sinks the entire basic block containing a conditional branch
forming a header of a selection construct, without accounting for some values
being used outside. This manifests as (for example):
```
<unknown>:0: error: 'spirv.Variable' op failed control flow structurization: it has uses outside of the enclosing selection/loop construct
<unknown>:0: note: see current operation: %0 = "spirv.Variable"()<{storage_class = #spirv.storage_class<Function>}> : () -> !spirv.ptr<vector<4xf32>, Function>
```
The proposed solution to this problem is to split the conditional basic block
into two, one block containing just the conditional branch, and other the rest
of instructions. By doing this, the logic that structures selection regions,
only sinks the comparison, keeping the rest of instructions outside the
selection region.
A SPIR-V test is required, as the problem can happen only during
deserialization and cannot be tested with `--test-spirv-roundtrip`. An MLIR
test exhibiting the problematic behaviour would be an incorrect MLIR in the
first place.
This solution is proposed as an alternative to an unfinished PR llvm#123371, that
is unlikely to be merged in the foreseeable future, as the author "stepped away
from this for a time being". There is also a Discourse thread:
https://discourse.llvm.org/t/spir-v-uses-outside-the-selection-region/84494
that tried to solicit some feedback on the topic.
IgWod-IMG
added a commit
to imaginationtech/llvm-project
that referenced
this pull request
Feb 24, 2025
With the current design some of the values are sank into a selection region,
despite them being also used outside that region. This is because the current
deserializer logic sinks the entire basic block containing a conditional branch
forming a header of a selection construct, without accounting for some values
being used outside. This manifests as (for example):
```
<unknown>:0: error: 'spirv.Variable' op failed control flow structurization: it has uses outside of the enclosing selection/loop construct
<unknown>:0: note: see current operation: %0 = "spirv.Variable"()<{storage_class = #spirv.storage_class<Function>}> : () -> !spirv.ptr<vector<4xf32>, Function>
```
The proposed solution to this problem is to split the conditional basic block
into two, one block containing just the conditional branch, and other the rest
of instructions. By doing this, the logic that structures selection regions,
only sinks the comparison, keeping the rest of instructions outside the
selection region.
A SPIR-V test is required, as the problem can happen only during
deserialization and cannot be tested with `--test-spirv-roundtrip`. An MLIR
test exhibiting the problematic behaviour would be an incorrect MLIR in the
first place.
This solution is proposed as an alternative to an unfinished PR llvm#123371, that
is unlikely to be merged in the foreseeable future, as the author "stepped away
from this for a time being". There is also a Discourse thread:
https://discourse.llvm.org/t/spir-v-uses-outside-the-selection-region/84494
that tried to solicit some feedback on the topic.
kuhar
pushed a commit
that referenced
this pull request
Feb 24, 2025
…127639) With the current design some of the values are sank into a selection region, despite them being also used outside that region. This is because the current deserializer logic sinks the entire basic block containing a conditional branch forming a header of a selection construct, without accounting for some values being used outside. This manifests as (for example): ``` <unknown>:0: error: 'spirv.Variable' op failed control flow structurization: it has uses outside of the enclosing selection/loop construct <unknown>:0: note: see current operation: %0 = "spirv.Variable"()<{storage_class = #spirv.storage_class<Function>}> : () -> !spirv.ptr<vector<4xf32>, Function> ``` The proposed solution to this problem is to split the conditional basic block into two, one block containing just the conditional branch, and other the rest of instructions. By doing this, the logic that structures selection regions, only sinks the comparison, keeping the rest of instructions outside the selection region. A SPIR-V test is required, as the problem can happen only during deserialization and cannot be tested with `--test-spirv-roundtrip`. An MLIR test exhibiting the problematic behaviour would be an incorrect MLIR in the first place. This solution is proposed as an alternative to an unfinished PR #123371, that is unlikely to be merged in the foreseeable future, as the author "stepped away from this for a time being". There is also a Discourse thread: https://discourse.llvm.org/t/spir-v-uses-outside-the-selection-region/84494 that tried to solicit some feedback on the topic.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.