Reflection issue fix

[RuoyuAMD]

1. Reserve unused std140 uniform block in reflection, and fix the uniform block struct member not affected by block matrix layout and memory layout.

According to the spec OpenGL spec4.60.7:

7.6 Uniform Variables:

All members of a named uniform block declared with a shared or std140 layout qualifier are considered active, even if they are not referenced in any shader in the program. The uniform block itself is also considered active, even if no member of the block is referenced.

We should reserve std140 block and shared block in reflection.

According to the spec glsl4.60.7:

4.4.5. Uniform and Shader Storage Block Layout Qualifiers:

The row_major and column_major qualifiers only affect the layout of matrices, including all
matrices contained in structures and arrays they are applied to, to all depths of nesting. These qualifiers can be applied to other types, but will have no effect.

We need ensure all matrix block member been effect, include the member of a struct in this block.

2. Support EShMsgKeepUncalled in reflection.

EShMsgKeepUncalled is a link message for link program. We need only one option to control uncalled function optimization. If we set EShMsgKeepUncalled as false in link time, linker won't be keep the uncall function sequence in AST, and if we set EShMsgKeepUncalled as true in link time, linker will keep all uncalled function sequence in AST.

2. Recursively layout packing to "block member".

Layout packing isn't set recursively, it causes TReflection::getOffsets doesn't work correctly.

3. Recursively layout matrix to "block member".

Layout matrix isn't set recursively, it causes a matrix layout incorrectly issue.

So, in reflecte time, we just only travers all function sequence. It make EShMsgKeepUncalled only work at linker, and can effect reflection.

[johnkslang]

I afraid that GLSL reflection is mistakenly getting used as a substitute for SPIR-V reflection.

I think there are tools out there to help with SPIR-V reflection.

At a high-level, the same struct can be used in multiple places with contradictory layouts, implying two lower-level structs with different layouts to back the one high-level/abstract struct.

GLSL and the AST are higher level, representing all this with one struct.

SPIR-V is lower level, expanding this one struct to two structs.

[johnkslang]

This is incorrect.

The same struct can be used in multiple places with contradictory layouts, implying two lower-level structs with different layouts to back the one high-level/abstract struct.

GLSL and the AST are higher level, representing all this with one struct. SPIR-V is lower level, expanding this one struct to two structs.

I'm afraid you're trying to apply a low-level transform to a high-level representation.

[RuoyuAMD]

I updated the code. Now when fix layoutMatrix and layoutPacking will clone a new struct, and will declare a new struct in Spirv code too. But I'm worry it will affect decompiling. because this structs in spirv have the same name, we make work out a way to give them a mangle name; like:
BlockName@structName .

[johnkslang]

But, it is not correct at GLSL or AST level to have duplicate structures. They really are the same type at the high level, but different types at the SPIR-V level.

SPIR-V was already doing the duplication, and I think the SPIR-V code was already correct, yes?

[johnkslang]

Was there already an existing problem with GLSL/AST level starting to push information into nested structures, but incorrectly? It seems there are two paths here:

A. fix it from doing that, at the high level, there should be only one
B. do it more, completely, requiring structure copying, which is a pretty major change

So, I guess three questions here:

1. is it true this issue was present? (I can believe it from the memory qualifier test)
2. is it the case you are choosing path B instead of fixing the code to match the design with path A?
3. What are all the implications of changing GLSL/AST from single copy to multiple copy of the same struct?

It is possible you are fixing real problems here, but I worry it is by changing design and interface, and wonder about path A as well.

[RuoyuAMD]

Was there already an existing problem with GLSL/AST level starting to push information into nested structures, but incorrectly? It seems there are two paths here:

A. fix it from doing that, at the high level, there should be only one
B. do it more, completely, requiring structure copying, which is a pretty major change

So, I guess three questions here:

1. is it true this issue was present? (I can believe it from the memory qualifier test)
2. is it the case you are choosing path B instead of fixing the code to match the design with path A?
3. What are all the implications of changing GLSL/AST from single copy to multiple copy of the same struct?

It is possible you are fixing real problems here, but I worry it is by changing design and interface, and wonder about path A as well.

Yes, there is a existing problem with GLSL/AST level.

1. is it true this issue was present?
   • This issue was present.
2. is it the case you are choosing path B instead of fixing the code to match the design with path A?
   • I chooise the path B to fix it.
3. What are all the implications of changing GLSL/AST from single copy to multiple copy of the same struct?
   • This will result in multiple copies of the structure in memory in addition to the original structure, but these copies are unique if they have the same memory layout or the same matrix layout. That's vary important, it can help we not generate duplicate struct in spirv code. So, it won't implicate spirv generate. One practical effect it may bring is that when we want to modify only the origin structure to affect all struct used in block, we may not get the results we want. If we want to affect all struct used in block, we need to process all the structural branches in the record.

In fact, I chose path B to reduce the impact on the design and interface. If we want fix it in high level, we have to modify the TTypeLoc (path A), and save layout information, matrix information in it, we have discussed internally, I think this is more fit, but the risk will be higher. path B won't impact interface, we only make a struct copy in AST, When we choose path B, this is the only way. If the behavior of some interfaces is affected, I think this is also easy to fix, just apply the behavior of the interface to all records.

[johnkslang]

But, it is not correct at GLSL or AST level to have duplicate structures. They really are the same type at the high level, but different types at the SPIR-V level.

SPIR-V was already doing the duplication, and I think the SPIR-V code was already correct, yes?

[johnkslang]

There has been little discussion or answers about the biggest thing happening here.

• Currently, AST, GLSL, and HLSL share one high-level structure for multiply physical layouts.
• Currently, SPIR-V uses multiple structures for a single high-level structure, one for each used physical layout.

It's a pretty big change to instead have the AST in the SPIR-V bucket instead of the GLSL and HLSL bucket. If this major architectural change is proposed, we should:

• be clear what the new architecture and high-level design is
• think deeply about what might break and add new tests for it
• get consensus this is how we want to change glslang operation

Maybe this is okay to do, but it should not just be a side effect of seeing a bug in code reading. It should be a proactively broadcast design change.

[RuoyuAMD]

There has been little discussion or answers about the biggest thing happening here.

• Currently, AST, GLSL, and HLSL share one high-level structure for multiply physical layouts.
• Currently, SPIR-V uses multiple structures for a single high-level structure, one for each used physical layout.

It's a pretty big change to instead have the AST in the SPIR-V bucket instead of the GLSL and HLSL bucket. If this major architectural change is proposed, we should:

• be clear what the new architecture and high-level design is
• think deeply about what might break and add new tests for it
• get consensus this is how we want to change glslang operation

Maybe this is okay to do, but it should not just be a side effect of seeing a bug in code reading. It should be a proactively broadcast design change.

I agree that, and I think the most important is that we must be clear what the new architecture and high-level design is.

1. Currently, AST, GLSL, and HLSL share one high-level structure for multiply physical layouts. ----- I agree.

2. Currently, SPIR-V uses multiple structures for a single high-level structure, one for each used physical layout.
   ---- I agree.

3. instead have the AST in the SPIR-V bucket instead of the GLSL and HLSL bucket.
   ---- I'm not agree.

Ideally, AST should be closer to the semantics of high-level languages, rather than to accommodate the structure of low-level languages. but, why do we encounter such problems?
I think it's a Typelist design issue. We use type list to contian the struct members details, but layoutPacking and layoutMartrix is not the struct member detail, it's a block attribute, even this attribute are declare on block member, but it still a block attribute. so I think a better representation is move qualifie form TType to TTypeLoc, just like loc:

rename :
TTypeLoc -> TTypeDetail

and :
typedef TVector TTypeList;

struct TTypeDetail {
    TType* type;
    TQualifier qualifier
    TSourceLoc loc;
};

To make a separation of type and type qualifier and use TTypeDetail to instead the TType in symbol node. And it can help us solve problems from a theoretical level rather than an engineering level.

So I think the root cause is not share one high-level structure for multiply physical layouts, the root cause is we put two different things together in AST.

But it a huge change. This will be a huge contraction and bring about possible refactoring. We need to adjust in many places. It can be a long-term plan, but currently we need to solve this problem engineeringly, and cloning a new structure is the most convenient effective method. The biggest uncertainty brought by this solution is that the change of debugging information leads to more inaccurate decompiled results.

We are frequently developing and integrating on glslang. When new problems are exposed, we will follow up and fix them in a timely manner. Because this change caused a big change in the generated spirv, I had some negligence when I checked, and I am sorry for this. But the big changes also prove that our test cases cover a very wide range, and I'm glad to have such a comprehensive test and a careful reviewer like you. So don't worry too much, git gives us a truth: brave to modify, roll back if bad.

[RuoyuAMD]

Hi John,

We have other development and refinement plans, so I hope this pr can be merged as soon as possible, if nothing else.b @johnkslang

Thanks
Ruoyu

[RuoyuAMD]

Hi @johnkslang ,

Best wishes for you. beside that, could this change been merge to master now?
If the code doesn't have other issue, I hope it can be merged as soon as possible, which will make our next development plan, and code management more convenient and fluid.

Thanks
Ruoyu

[RuoyuAMD]

Hi John,
@johnkslang we have other Pr depend on this change, can we just merge it? if it has any issue, we can fixed it immediately.

Thanks
Ruoyu

[johnkslang]

The currently open "Conversation" issue should be addressed somehow. I view this as an example issue that it already has that needs a fix. Repeating here, but hopefully we can discuss in the issue:

Glslang did already expand these out. The source for this test has 3 needs for MyStruct:

• in a std430 SSBO
• in a std140 UBO
• as function local variable

The original test result has 3 instances of this type; at ids 12, 16, and 32.

The new test results have 6 instances of the type. See ids 12, 16, 26, 35, 51, and 62.

This appears to take glslang from doing the optimal thing with this type to doubling the number of types. Can you point out the bug in the existing test result?

[RuoyuAMD]

The currently open "Conversation" issue should be addressed somehow. I view this as an example issue that it already has that needs a fix. Repeating here, but hopefully we can discuss in the issue:

Glslang did already expand these out. The source for this test has 3 needs for MyStruct:

in a std430 SSBO
in a std140 UBO
as function local variable

The original test result has 3 instances of this type; at ids 12, 16, and 32.

The new test results have 6 instances of the type. See ids 12, 16, 26, 35, 51, and 62.

This appears to take glslang from doing the optimal thing with this type to doubling the number of types. Can you point out the bug in the existing test result?

This issue have been fixed in those commit, and the solution is create a map to record the structure copy, at first time, we create a structure copy as a temp. And fix it's layoutPacking or layoutMatrix on the temp structure, after that, we will check this 2 qualifier whether been changed, if it been changed, we will use the copied structure replace the original structure.

Based on this design, in order to ensure that our records can be properly recorded and recursive, layoutMatrix will also be recorded in the TType of the structure, which is why some case's AST dump has changed, in which more layout declarations came out. But I think it's expected and it's correct, It can also make sure TQualifier::isMatrix can return a correct result.

Thanks
Ruoyu

[RuoyuAMD]

What is the progress of the review, do you have any other suggestions? I hope this Pr can be merged into the master as soon as possible. It's a huge change, and our work been blocked on it.
If there are any other problems after merging the code, we will fix them as soon as possible.
@johnkslang

[RuoyuAMD]

Hi John,
@johnkslang . We are still waiting for this change to merge to ensure that our other development based on this change can be completed as soon as possible. Now that this pr cannot be merged has affected our project. I hope you can speed up the review. If there are other problems, I will fix them as soon as possible. If there are still problems after the code is merged, you can roll back or notify me to fix them as soon as possible. Putting this Pr on hold is not a wise decision. In fact, it fixes a glslang hidden problem.

Best Regards,
Ruoyu

[johnkslang]

Yes, I am actually reviewing now. Hope to finish today.

[johnkslang]

Sorry this is taking so long, part of that is my fault. For other parts, it will be faster to handle PRs when:

• they are smaller; I think this one got better
• if a reviewer raises an issue, let the reviewer close the conversation after reading and agreeing with the resolution (what happened here a few times is you answered and then closed the conversation, so I did not see the answer, or sometimes also did not agree to it, but it was somewhat hidden)

[johnkslang]

There are still a few genuine concerns here.

[johnkslang]

The point of the test was to ensure that unused_uniform was not reflected, and that fsonly_uniform was only reflected for the fragment stage.

Is there a reason to be breaking this?

I'm pretty sure some users depend on reflection not reporting unused items. There are separate options to turn on to get more information.

[RuoyuAMD]

The point of the test was to ensure that unused_uniform was not reflected, and that fsonly_uniform was only reflected for the fragment stage.

Is there a reason to be breaking this?

I'm pretty sure some users depend on reflection not reporting unused items. There are separate options to turn on to get more information.

According to the spec OpenGL spec4.60.7:

7.6 Uniform Variables:

All members of a named uniform block declared with a shared or std140 layout qualifier are considered active, even if they are not referenced in any shader in the program. The uniform block itself is also considered active, even if no member of the block is referenced.

layout(binding = 0, std140) uniform ubo_block {
	float unused_uniform;
	float shared_uniform;
	float vsonly_uniform;
	float fsonly_uniform;
} ubo;

If app user don't want compiler reporting unused uniform block, they should use std430 layout.
If gpu driver user don't want compiler reporting unused uniform block, they can implement a reflection out of glslang, and travers again. In fact, when we encounter similar problems, we do just that. Glslang should correctly based on the spec, and venders needs to find a way to meet its own needs.

[johnkslang]

Reflection isn't a normative specification thing, it as a tooling thing added on by users that needed that specific tooling.

Khronos does not have a specification for this tooling feature.

Maybe you need something similar, but need to add what it is you need, not re-specify and break something that people are already using.

[RuoyuAMD]

Reflection isn't a normative specification thing, it as a tooling thing added on by users that needed that specific tooling.

Khronos does not have a specification for this tooling feature.

Maybe you need something similar, but need to add what it is you need, not re-specify and break something that people are already using.

You can't use "Reflection isn't a normative specification thing" to deny that reflection needs to abide by glsl norms. The compiler is just a tooling thing too, but since this compiler is a glsl compiler, it should be subject to the glsl norms, the same Since reflection is glsl's reflection, it should also be subject to glsl's specifications, otherwise it is not glsl's reflection, or you should rename it to someotherReflection. So, I think glsl should honor to the spec, not to mention that I have not seen negative comments or questions from users under this Pr. Bringing people more standard tools is not a break.
The changes we have made are based on the test results from CTS. There are sufficient and legitimate reasons for glslang to respect and restore the provisions of the spec from all angles.

[johnkslang]

Was there already an existing problem with GLSL/AST level starting to push information into nested structures, but incorrectly? It seems there are two paths here:

A. fix it from doing that, at the high level, there should be only one
B. do it more, completely, requiring structure copying, which is a pretty major change

So, I guess three questions here:

1. is it true this issue was present? (I can believe it from the memory qualifier test)
2. is it the case you are choosing path B instead of fixing the code to match the design with path A?
3. What are all the implications of changing GLSL/AST from single copy to multiple copy of the same struct?

It is possible you are fixing real problems here, but I worry it is by changing design and interface, and wonder about path A as well.

[RuoyuAMD]

Sorry this is taking so long, part of that is my fault. For other parts, it will be faster to handle PRs when:

• they are smaller; I think this one got better
• if a reviewer raises an issue, let the reviewer close the conversation after reading and agreeing with the resolution (what happened here a few times is you answered and then closed the conversation, so I did not see the answer, or sometimes also did not agree to it, but it was somewhat hidden)

The code of this change itself is not very large, but it affects a lot of cases, I did not do enough checks, so a lot of errors occurred at the beginning (Self-confidence is a good thing , but I should not check it when it affects so many cases :)).
In AMD, We use review board, when I fixed a issue, I will click button [Fixed] to mark it, I thought resolve button same as it, but it's OK, I will keep the state in unsolved in future.
You may be able to invite more contributors to help you view the code, such as Rexu.

I can't blame you. Actually I am very worried about your life and hope you pay attention to safety and stay healthy during COV-19, no one would like to lose a great engineer / scientist. If you need any help from china, you may contect me.
Best wish to you.

[johnkslang]

First, despite my strict review, I want to thank you for doing much hard work here and figuring all this out. I am sure you have found important improvements, and thank you for that.

I think there are two things going on here:

1. there are some bugs with nested structures that you found fixes for
2. you want to change definition of how reflection works

The first one is great, while the second one is worrisome.

A few paths forward:

A) separate this into multiple PRs that are clearly fixes versus behavior changes independent of the fixes,
B) or, champion getting approval of downstream users for the semantic changes you want to make,
C) or, put your semantic changes behind a new option, such that default operation remains the same.

[RuoyuAMD]

First, despite my strict review, I want to thank you for doing much hard work here and figuring all this out. I am sure you have found important improvements, and thank you for that.

I think there are two things going on here:

1. there are some bugs with nested structures that you found fixes for
2. you want to change definition of how reflection works

The first one is great, while the second one is worrisome.

A few paths forward:

A) separate this into multiple PRs that are clearly fixes versus behavior changes independent of the fixes,
B) or, champion getting approval of downstream users for the semantic changes you want to make,
C) or, put your semantic changes behind a new option, such that default operation remains the same.

About A:
The code review cycle is too long, I hope that these codes will be merged as soon as possible, so that we can mention another Pr to achieve C.

About B:
I think CTS is the best interpretation of B.

About C:
Actually, We have added options for them, which have been implemented in our local changes, but because this change has not been merged into, we have not submitted the relevant Pr for these codes, this is some very huge changes, when this change After being merged , we will immediately organize the local code and submit it. These codes are scattered in multiple changes, in order to facilitate our local better management of them.

[johnkslang]

It is partly my fault, but also the code review cycle is long partly because of the size of the original changes, and because each review finds non-backward-compatible changes.

Smaller PRs without backward incompatible changes can progress much faster.

I'm not understanding why we can't incrementally make the fixes without breaking existing users.

I did not know CTS was measuring this. CTS tests drivers, not glslang reflection, yes?

Glslang reflection was a side-band tool, for a particular GLSL purpose, not a legally normative implementation of any specification.

Can you help us understand better?

[RuoyuAMD]

Those case are under the influence of EShMsgKeepUncalled option, this option did not work in old implementation, but now it can work normally. Therefore, some changes have taken place when traversing the grammar tree, which will result in the change of some variables and index used in EShMsgKeepUncalled.

[RuoyuAMD]

@johnkslang ,I rebased all commit for you easily review code.

[RuoyuAMD]

It is partly my fault, but also the code review cycle is long partly because of the size of the original changes, and because each review finds non-backward-compatible changes.

Smaller PRs without backward incompatible changes can progress much faster.

I'm not understanding why we can't incrementally make the fixes without breaking existing users.

I did not know CTS was measuring this. CTS tests drivers, not glslang reflection, yes?

Glslang reflection was a side-band tool, for a particular GLSL purpose, not a legally normative implementation of any specification.

Can you help us understand better?

I add a option for disable this reflection, please review again, if no problem, after test passed, you can merge it now.