-
Notifications
You must be signed in to change notification settings - Fork 819
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.
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
Fix issue for new unique id system. #2458
Conversation
The root cause is within TGlslangToSpvTraversery:visitSymbol(), rValueParameters needs to check unique id in symbol table. But in function ProcessDeferred() (ShaderLang.cpp), symbolTable->adoptLevels(*cacheTable) will reset the unique ID, using max id in cache at that moment, and previous ids will be no longer effective. So the fix is to re-set those ids following above call. Besides, it is needed to explicitly using setUniqueId() before parse() currently. Which could ensure id has been initialized successfully. |
Thanks, trying this out against my case that's currently failing |
Would you mind to attach example shaders showing the case we discussed before here? So that I could debug with it. |
The shaders that it's failing on requires: |
Ok, I see those shaders added in latest commit. It is ok, I can have a try if above issue still cause failing. |
The shaders that show the error aren't in the test cases actually. The ones I see failing are far more complex. This fix doesn't solve the issue though. The error occurs during the link stage when the multiple compilation units are merged together into a single one. Doesn't this fix occur before that? The fact that the new updateUniqueIdLevelFlag() clamps at a max of 7 seems questionable to me. Why not just increase the uniqueIds to 64-bit. Give more bits for the 'level', and then some other bits can be used for the compilation unit index when doing the merging, which would make things unique. |
I agreed to increase the uniqueIds to 64-bits. Three bits for levels may lead to issues of same unique id, as built-in symbols will also use some of them. Currently I don't have a test case to re-produce this issue and verify my fix. I can make a change for this but can't make sure whether it could fix your local problems. PS. This PR is for the problem mentioned in comments. We could create another PR for 64-bits uniqueIds. |
I'll be happy to test any fix you make. I can also try to pull out the sample shaders, it's just a little tricky cause they are procedurally generated in code. |
I fully understand that. It would be much helpful if I could get some simple sample shaders for local verifying. Anyway, I'll make another change for this, expanding uniqueIds to 64 bits. |
This is the opposite of a minimal case, but it does reproduce it if you are using the vulkan_relaxed extension in the above branch. |
I'll have a try. |
Hey @ShchchowAMD, any chance to look at this again? Thanks |
Hi mbechard, I'll try to fix above issue this week. BR. |
Any update on this? It sounds like it might relate to the recent issue #2495. |
So to confirm: it is believed that this change fixes a previous problem and does not cause any regressions. |
Merging multiple compilation units is still broken because the ids being assigned to variables are not made unique during the merging. The discussion in this pull describes the potential solution: upping the ids to 64-bit, reserving some bits for the module #, some of the level # and the rest as the ID. |
Yes, but while we are waiting for the 64-bit fix, I am wondering if there is benefit (and no harm) in merging the current change. We could open another issue pointing at this PR and the outstanding issue you have outlined. |
I'm not sure if there were other issues that ShchchowAMD was looking to fix with this. I think this PR was meant to fix a regression I brought up in PR #2396, but it doesn't quite do the trick in it's current form. |
When I changed it to 64 bits (currently I set 8 bits for level), I met some issues in GlslangToSPIRV. One of them is : some variables are compiled to wrong types (seems to be a new issue caused by id), I am now debugging with those issues. |
This new commit seems to fix the issue for all of test cases I'm running through. Thanks @ShchchowAMD ! |
This PR fixes #2509 Thanks @ShchchowAMD ! |
@greg-lunarg Hi greg, I think this would be all for fixing issues mentioned above. Please take a look whether it is ok to merge, Thanks! |
@ShchchowAMD Will review presently. Thanks! |
Yes, if the max() issue is fixed I think the current solution does fix the current issues. |
@mbechard Are you suggesting that one more fix is required before these commits can be pushed, or that these commits can be pushed now even though a final fix will be needed afterwards to fix all issues? |
In it's current state this PR still has one defect I see, which is the std::max() line issue I pointed out in the review note. If you're looking to merge this ASAP, then this PR is certainly better than the current state of the master branch, so go ahead. |
@mbechard Do these recent two commits address the remaining problems? |
Yep looks like it. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one concern regarding new(?) gap in slot assignment.
This all looks good. Could you please squash into one commit? Then I will merge. |
@ShchchowAMD Can you please squash your 8 commits down to 1 so I can merge? Thanks! |
Fixes #2495 |
Fixes #2509 |
OK |
…symbols and split symbol tables. For intermediates rebuilding, now need manually amending level bits for redeclaring built-ins.
93eb634
to
93b400f
Compare
I believe the failed ubuntu-clang-gn was due to how far behind the branch was. I am going ahead with merge. |
ubuntu-clang-gn pass confirmed with #2534 |
No description provided.