[SPIR-V] Disable Machine Sink pass in SPIR-V Backend #116060
Merged
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
|
@llvm/pr-subscribers-backend-spir-v Author: Vyacheslav Levytskyy (VyacheslavLevytskyy) ChangesSome standard passes that optimize machine instructions in SSA form uses MI.isPHI() that doesn't account for OpPhi in SPIR-V and so are able to break the CFG. MachineSink is among such passes (see for example llvm-project/llvm/lib/CodeGen/MachineSink.cpp Line 630 in 1884ffc There is a reproducer of the issue that demonstrates how MachineSink is able to generate an invalid code for the SPIR-V Backend The reproducer is a part of SYCL end-to-end test suite (https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/DeviceLib/imf_fp32_rounding_test.cpp). At the moment it doesn't seem feasible to make it a part of the SPIR-V Backend test suite due to a far too big size of the intermediate LLVM IR that causes the problem. Full diff: https://github.com/llvm/llvm-project/pull/116060.diff 1 Files Affected:
diff --git a/llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp b/llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp
index 194ce7c10bfd3f..bf4d974329de34 100644
--- a/llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp
@@ -102,6 +102,7 @@ class SPIRVPassConfig : public TargetPassConfig {
SPIRVTargetMachine &getSPIRVTargetMachine() const {
return getTM<SPIRVTargetMachine>();
}
+ void addMachineSSAOptimization() override;
void addIRPasses() override;
void addISelPrepare() override;
@@ -129,6 +130,16 @@ FunctionPass *SPIRVPassConfig::createTargetRegisterAllocator(bool) {
return nullptr;
}
+// Disable passes that may break CFG.
+void SPIRVPassConfig::addMachineSSAOptimization() {
+ // Some standard passes that optimize machine instructions in SSA form uses
+ // MI.isPHI() that doesn't account for OpPhi in SPIR-V and so are able to
+ // break the CFG (e.g., MachineSink).
+ disablePass(&MachineSinkingID);
+
+ TargetPassConfig::addMachineSSAOptimization();
+}
+
// Disable passes that break from assuming no virtual registers exist.
void SPIRVPassConfig::addPostRegAlloc() {
// Do not work with vregs instead of physical regs.
|
|
@michalpaszkowski @Keenuts This PR is a reminder that #110507 doesn't resolve all issues related to OpPhi in SPIR-V. |
Probably I would make it a part of a wider initiative, with a RFC and proper discussion. |
michalpaszkowski
approved these changes
Nov 19, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/88/builds/4774 Here is the relevant piece of the build log for the reference |
VyacheslavLevytskyy
added a commit
that referenced
this pull request
Dec 9, 2024
…9202) This PR improves general validity of emitted code between passes due to generation of `TargetOpcode::PHI` instead of `SPIRV::OpPhi` after Instruction Selection, fixing generation of OpTypePointer instructions and using of proper virtual register classes. Using `TargetOpcode::PHI` instead of `SPIRV::OpPhi` after Instruction Selection has a benefit to support existing optimization passes immediately, as an alternative path to disable those passes that use `MI.isPHI()`. This PR makes it possible thus to revert #116060 actions and get back to use the `MachineSink` pass. This PR is a solution of the problem discussed in details in #110507. It accepts an advice from code reviewers of the PR #110507 to postpone generation of OpPhi rather than to patch CodeGen. This solution allows to unblock improvements wrt. expensive checks and makes it unrelated to the general points of the discussion about OpPhi vs. G_PHI/PHI. This PR contains numerous small patches of emitted code validity that allows to substantially pass rate with expensive checks. Namely, the test suite with expensive checks set ON now has only 12 fails out of 569 total test cases. FYI @bogner
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Some standard passes that optimize machine instructions in SSA form uses MI.isPHI() that doesn't account for OpPhi in SPIR-V and so are able to break the CFG. MachineSink is among such passes (see for example
llvm-project/llvm/lib/CodeGen/MachineSink.cpp
Line 630 in 1884ffc
There is a reproducer of the issue that demonstrates how MachineSink is able to generate an invalid code for the SPIR-V Backend
The reproducer is a part of SYCL end-to-end test suite (https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/DeviceLib/imf_fp32_rounding_test.cpp). At the moment it doesn't seem feasible to make it a part of the SPIR-V Backend test suite due to a far too big size of the intermediate LLVM IR that causes the problem.