[Mesa-dev] [PATCH] glsl: Do not propagate 'precise' and 'invariant' on read-only variables

[Danylo Piliaiev]

Hi Jason,

On 8/22/18 7:07 PM, Jason Ekstrand wrote:
> I think this would be correct to do and would fix the bug at hand but 
> there may be other bugs lurking deeper.  In particular, if you have 
> multiple shaders for the same stage that share some global variables 
> and get linked together, I think you could run into the same issue 
> only with a read-write global.  If this is indeed possible, then I'm 
> not sure what to do.  We have to run this pass prior to doing any 
> optimization because we depend on it to get precise and invariant 
> correct.  On the other hand, it has to be run after linking. :-/  
> Maybe we just have to assume precise and invariant for all pre-linking 
> optimizations and then run this post-linking.

I didn't check this case, thanks for pointing it out. Now I have looked 
at relevant code and saw that invariant propagation (linker.cpp:5024 in 
linker_optimisation_loop) is done after 'link_intrastage_shaders' 
(linker.cpp:4868) which combines a group of shaders for a single stage. 
Actually invariant propagation is also done once before linking. In 
addition I've done some tests and invariance from variable in one vertex 
shader is propagated on variables directly used only in other vertex 
shader.

However I have found other issue - cross_validate_globals is called in 
link_intrastage_shaders and throws error if 'invariant' qualifier 
doesn't match between shaders in same stage which happens when variable 
in one shader becomes invariant before the 'link_intrastage_shaders' 
call. This could be fixed separately.

Or there is another way to solve these issues: divide 'invariant' 
qualifier into explicit and implicit 'invariant', where explicit is for 
'invariant' set by user in shader and implicit is for 'invariant' 
propagated from other invariants. Thus in interface checking we will be 
able to check only for explicit invariance which besides two previous 
issues with solve another one: if out variable is marked as invariant 
due to invariance propagation this variable will be required to have 
invariant qualifier in the next stage even though user never marked it 
as such anywhere. And when the invariance will actually matter we will 
check both implicit and explicit one. What do you think about such solution?

- Danil.

>
> On Wed, Aug 22, 2018 at 7:51 AM Danylo Piliaiev 
> <danylo.piliaiev at gmail.com <mailto:danylo.piliaiev at gmail.com>> wrote:
>
>     Read-only variables could be considered inherently invariant and
>     precise
>     since no expression in shader affects them.
>     Explicitly marking them as such is unnecessary and can cause issues,
>     e.g. uniform marked as invariant may require the same uniform in other
>     stage to be invariant too but uniforms cannot have "invariant"
>     qualifier.
>
>     Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100316
>
>     Signed-off-by: Danylo Piliaiev <danylo.piliaiev at globallogic.com
>     <mailto:danylo.piliaiev at globallogic.com>>
>     ---
>     My assumption is that variable being read_only means that it either
>     loads a value from an external source or is initialized from
>     constant value.
>     From what I saw in code it's true but it may be wrong to depend on it.
>
>      src/compiler/glsl/propagate_invariance.cpp | 10 ++++++++++
>      1 file changed, 10 insertions(+)
>
>     diff --git a/src/compiler/glsl/propagate_invariance.cpp
>     b/src/compiler/glsl/propagate_invariance.cpp
>     index b3f1d810cd..0e4233aa9f 100644
>     --- a/src/compiler/glsl/propagate_invariance.cpp
>     +++ b/src/compiler/glsl/propagate_invariance.cpp
>     @@ -96,6 +96,16 @@
>     ir_invariance_propagation_visitor::visit(ir_dereference_variable *ir)
>         if (this->dst_var == NULL)
>            return visit_continue;
>
>     +   /* Read-only variables could be considered inherently
>     +    * invariant and precise since no expression in shader affects
>     them.
>     +    * Explicitly marking them as such is unnecessary and can cause
>     +    * issues, e.g. uniform marked as invariant may require the same
>     +    * uniform in other stage to be invariant too but uniforms cannot
>     +    * have "invariant" qualifier.
>     +    */
>     +   if (ir->var->data.read_only)
>     +      return visit_continue;
>     +
>         if (this->dst_var->data.invariant) {
>            if (!ir->var->data.invariant)
>               this->progress = true;
>     -- 
>     2.18.0
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180823/b360c3ea/attachment.html>