| Bug ID | 90056 |
|---|---|
| Summary | Unigine Valley regression since radeon/llvm: Run LLVM's instruction combining pass |
| Product | Mesa |
| Version | git |
| Hardware | Other |
| OS | All |
| Status | NEW |
| Severity | normal |
| Priority | medium |
| Component | Drivers/Gallium/radeonsi |
| Assignee | dri-devel@lists.freedesktop.org |
| Reporter | adf.lists@gmail.com |
| QA Contact | dri-devel@lists.freedesktop.org |
R9270X Since commit c6d79ed289a75f13c65f011be870f7e43a0fedc7 Author: Tom Stellard <thomas.stellard@amd.com> Date: Fri Apr 10 17:07:16 2015 +0000 radeon/llvm: Run LLVM's instruction combining pass This should improve code quality in general and will help with some future changes to how we emit kill instructions. shader-db shows a few regressions, but these don't seem to be the result of deficiencies in instcombine. They're mostly caused by the scheduler making different decisions than before. Unigine Valley quits saying - Unhandled loop condition! UNREACHABLE executed at SIAnnotateControlFlow.cpp:267! Only tried my normal settings on valley which are quality ultra + 8x AA
Could you run the program with the environment variable R600_DEBUG=ps,vs,gs and post the output.
Created attachment 115136 [details]
valley with R600_DEBUG=ps,vs,gs
Control flow annotation seems to choke on a predecessor with "undef" incoming value, for some reason.
Created attachment 115333 [details] [review] Reduced test case Here's a reduced testcase. I'm not sure how undef branch conditions are supposed to be handled, make someone else can take a look?
If this can't be fixed soon, maybe the bisected change should = be reverted for now?
Created attachment 115401 [details] [review] Possible fix How about this? Not tested yet, I don't have access to a radeon GPU right now.
(In reply to Grigori Goronzy from comment #6) > Created attachment 115401 [details] [review] [review] > Possible fix > > How about this? Not tested yet, I don't have access to a radeon GPU right > now. It doesn't help valley.
Does this patch help: http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150202/257738.html
Created attachment 115409 [details] [review] Full shader That does not help either. Seems to break the SSA somehow. Full shader that triggers the bug attached. The patch I posted earlier help with the the reduced testcase, but not with the full shader. Both undef incoming values in phi nodes and undef branch conditions cause problems in different ways.
(In reply to Grigori Goronzy from comment #9) > Created attachment 115409 [details] [review] [review] > Full shader > > That does not help either. Seems to break the SSA somehow. Full shader that > triggers the bug attached. > > The patch I posted earlier help with the the reduced testcase, but not with > the full shader. Both undef incoming values in phi nodes and undef branch > conditions cause problems in different ways. With the second patch + valley I get - valley_x64: LiveVariables.cpp:114: void llvm::LiveVariables::MarkVirtRegAliveInBlock(llvm::LiveVariables::VarInfo &, llvm::MachineBasicBlock *, llvm::MachineBasicBlock *, std::vector<MachineBasicBlock *> &): Assertion `MBB != &MF->front() && "Can't find reaching def for virtreg"' failed.
Created attachment 115423 [details] [review] Reduced test case
(In reply to Andy Furniss from comment #10) > (In reply to Grigori Goronzy from comment #9) > > Created attachment 115409 [details] [review] [review] [review] > > Full shader > > > > That does not help either. Seems to break the SSA somehow. Full shader that > > triggers the bug attached. > > > > The patch I posted earlier help with the the reduced testcase, but not with > > the full shader. Both undef incoming values in phi nodes and undef branch > > conditions cause problems in different ways. > > With the second patch + valley I get - > > valley_x64: LiveVariables.cpp:114: void > llvm::LiveVariables::MarkVirtRegAliveInBlock(llvm::LiveVariables::VarInfo &, > llvm::MachineBasicBlock *, llvm::MachineBasicBlock *, > std::vector<MachineBasicBlock *> &): Assertion `MBB != &MF->front() && > "Can't find reaching def for virtreg"' failed. Did test with only the patch from comment #8 or did you test with both patches?
(In reply to Tom Stellard from comment #12) > (In reply to Andy Furniss from comment #10) > > (In reply to Grigori Goronzy from comment #9) > > > Created attachment 115409 [details] [review] [review] [review] [review] > > > Full shader > > > > > > That does not help either. Seems to break the SSA somehow. Full shader that > > > triggers the bug attached. > > > > > > The patch I posted earlier help with the the reduced testcase, but not with > > > the full shader. Both undef incoming values in phi nodes and undef branch > > > conditions cause problems in different ways. > > > > With the second patch + valley I get - > > > > valley_x64: LiveVariables.cpp:114: void > > llvm::LiveVariables::MarkVirtRegAliveInBlock(llvm::LiveVariables::VarInfo &, > > llvm::MachineBasicBlock *, llvm::MachineBasicBlock *, > > std::vector<MachineBasicBlock *> &): Assertion `MBB != &MF->front() && > > "Can't find reaching def for virtreg"' failed. > > Did test with only the patch from comment #8 or did you test with both > patches? Just #8 but I just tried with both and get the same fail.
| What | Removed | Added |
|---|---|---|
| Attachment #115423 is obsolete | 1 |
Created attachment 115488 [details] [review] Possible fix Can you try this applying this patch only?
(In reply to Tom Stellard from comment #14) > Created attachment 115488 [details] [review] [review] > Possible fix > > Can you try this applying this patch only? This fixes it.
| What | Removed | Added |
|---|---|---|
| Status | NEW | RESOLVED |
| Resolution | --- | FIXED |
Since mesa 10.6 is going RC soon, what's going to happen with this feature? If it remains enabled, and the fix isn't backported to llvm 3.6, this bug will end up being present in a stable release (unless llvm 3.7 is also going to be released soon). Fez is also affected by the same bug (I tested and verified that it is fixed by the same patch), so there may be many more games affected: Good (mesa 10.5): https://www.dropbox.com/s/h7qtlgln1xlnxrk/fez_good.png?dl=0 Bad (mesa 10.6): https://www.dropbox.com/s/f5yqqx2l6pz284z/fez_bad.png?dl=0
(In reply to Furkan from comment #17) > Since mesa 10.6 is going RC soon, what's going to happen with this feature? > If it remains enabled, and the fix isn't backported to llvm 3.6, this bug > will end up being present in a stable release (unless llvm 3.7 is also going > to be released soon). > > Fez is also affected by the same bug (I tested and verified that it is fixed > by the same patch), so there may be many more games affected: > > Good (mesa 10.5): https://www.dropbox.com/s/h7qtlgln1xlnxrk/fez_good.png?dl=0 > Bad (mesa 10.6): https://www.dropbox.com/s/f5yqqx2l6pz284z/fez_bad.png?dl=0 This bug will be fixed in the LLVM 3.6.1 release.
