Bug 90056

Summary: Unigine Valley regression since radeon/llvm: Run LLVM's instruction combining pass
Product: Mesa Reporter: Andy Furniss <adf.lists>
Component: Drivers/Gallium/radeonsiAssignee: Default DRI bug account <dri-devel>
Status: RESOLVED FIXED QA Contact: Default DRI bug account <dri-devel>
Severity: normal    
Priority: medium CC: greg
Version: git   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: valley with R600_DEBUG=ps,vs,gs
Reduced test case
Possible fix
Full shader
Reduced test case
Possible fix

Description Andy Furniss 2015-04-16 21:45:51 UTC
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
Comment 1 Tom Stellard 2015-04-16 23:16:52 UTC
Could you run the program with the environment variable R600_DEBUG=ps,vs,gs and post the output.
Comment 2 Andy Furniss 2015-04-16 23:41:00 UTC
Created attachment 115136 [details]
valley with R600_DEBUG=ps,vs,gs
Comment 3 Grigori Goronzy 2015-04-17 07:22:48 UTC
Control flow annotation seems to choke on a predecessor with "undef" incoming value, for some reason.
Comment 4 Grigori Goronzy 2015-04-25 22:53:10 UTC
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?
Comment 5 Michel Dänzer 2015-04-28 08:22:45 UTC
If this can't be fixed soon, maybe the bisected change should be reverted for now?
Comment 6 Grigori Goronzy 2015-04-28 10:54:22 UTC
Created attachment 115401 [details] [review]
Possible fix

How about this? Not tested yet, I don't have access to a radeon GPU right now.
Comment 7 Andy Furniss 2015-04-28 13:03:01 UTC
(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.
Comment 8 Tom Stellard 2015-04-28 17:03:22 UTC
Does this patch help: http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150202/257738.html
Comment 9 Grigori Goronzy 2015-04-28 17:25:18 UTC
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.
Comment 10 Andy Furniss 2015-04-28 18:03:36 UTC
(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.
Comment 11 Tom Stellard 2015-04-29 02:04:25 UTC
Created attachment 115423 [details] [review]
Reduced test case
Comment 12 Tom Stellard 2015-04-29 02:06:43 UTC
(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?
Comment 13 Andy Furniss 2015-04-29 09:48:51 UTC
(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.
Comment 14 Tom Stellard 2015-04-30 21:19:03 UTC
Created attachment 115488 [details] [review]
Possible fix

Can you try this applying this patch only?
Comment 15 Andy Furniss 2015-04-30 22:17:03 UTC
(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.
Comment 16 Tom Stellard 2015-05-01 03:57:17 UTC
Fixed in llvm: r236306
Comment 17 Furkan 2015-05-11 21:38:24 UTC
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
Comment 18 Tom Stellard 2015-05-25 22:06:04 UTC
(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.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.