nir: fix compacting varyings when XFB outputs are present

Submitted by Samuel Pitoiset on Oct. 10, 2018, 7:44 a.m.

Details

Message ID 20181010074429.27313-1-samuel.pitoiset@gmail.com
State New
Headers show
Series "nir: fix compacting varyings when XFB outputs are present" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Samuel Pitoiset Oct. 10, 2018, 7:44 a.m.
We shouldn't try to compact any varyings known as always
active IO, especially XFB outputs. For example, if one
component of an xfb output is also used as input varying
in the next stage, it shouldn't be compacted.

Because we look at the input varyings from the consumer
stage, we don't know if one of them is an XFB output. One
solution is to mark all components as used when
always_active_io is true to avoid wrong remapping.

Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
---
 src/compiler/nir/nir_linking_helpers.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/compiler/nir/nir_linking_helpers.c b/src/compiler/nir/nir_linking_helpers.c
index 85712a7cb1..88014e9a1d 100644
--- a/src/compiler/nir/nir_linking_helpers.c
+++ b/src/compiler/nir/nir_linking_helpers.c
@@ -236,6 +236,15 @@  get_slot_component_masks_and_interp_types(struct exec_list *var_list,
                get_interp_type(var, default_to_smooth_interp);
             interp_loc[location + i] = get_interp_loc(var);
 
+            if (var->data.always_active_io) {
+               /* Mark all components as used to avoid repacting xfb varyings
+                * wrongly. For instance, if one component of an xfb output is
+                * also used as input varying in the next stage.
+                */
+               comps[location + i] |= 0xf;
+               continue;
+            }
+
             if (dual_slot) {
                if (i & 1) {
                   comps[location + i] |= ((1 << comps_slot2) - 1);

Comments

On 10/10/18 6:44 pm, Samuel Pitoiset wrote:
> We shouldn't try to compact any varyings known as always
> active IO, especially XFB outputs. For example, if one
> component of an xfb output is also used as input varying
> in the next stage, it shouldn't be compacted.
> 
> Because we look at the input varyings from the consumer
> stage, we don't know if one of them is an XFB output. One
> solution is to mark all components as used when
> always_active_io is true to avoid wrong remapping.
> 
> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
> ---
>   src/compiler/nir/nir_linking_helpers.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/src/compiler/nir/nir_linking_helpers.c b/src/compiler/nir/nir_linking_helpers.c
> index 85712a7cb1..88014e9a1d 100644
> --- a/src/compiler/nir/nir_linking_helpers.c
> +++ b/src/compiler/nir/nir_linking_helpers.c
> @@ -236,6 +236,15 @@ get_slot_component_masks_and_interp_types(struct exec_list *var_list,
>                  get_interp_type(var, default_to_smooth_interp);
>               interp_loc[location + i] = get_interp_loc(var);
>   
> +            if (var->data.always_active_io) {
> +               /* Mark all components as used to avoid repacting xfb varyings
> +                * wrongly. For instance, if one component of an xfb output is
> +                * also used as input varying in the next stage.
> +                */
> +               comps[location + i] |= 0xf;
> +               continue;
> +            }

In GLSL we specifically mark the corresponding inputs as 
always_active_io. I think we should try to do that for spirv also 
otherwise we are going to run into issues with the other lowering passes 
that check always_active_io such as scalar/array lowering.

> +
>               if (dual_slot) {
>                  if (i & 1) {
>                     comps[location + i] |= ((1 << comps_slot2) - 1);
>
On 10/10/18 10:14 AM, Timothy Arceri wrote:
> On 10/10/18 6:44 pm, Samuel Pitoiset wrote:
>> We shouldn't try to compact any varyings known as always
>> active IO, especially XFB outputs. For example, if one
>> component of an xfb output is also used as input varying
>> in the next stage, it shouldn't be compacted.
>>
>> Because we look at the input varyings from the consumer
>> stage, we don't know if one of them is an XFB output. One
>> solution is to mark all components as used when
>> always_active_io is true to avoid wrong remapping.
>>
>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
>> ---
>>   src/compiler/nir/nir_linking_helpers.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/src/compiler/nir/nir_linking_helpers.c 
>> b/src/compiler/nir/nir_linking_helpers.c
>> index 85712a7cb1..88014e9a1d 100644
>> --- a/src/compiler/nir/nir_linking_helpers.c
>> +++ b/src/compiler/nir/nir_linking_helpers.c
>> @@ -236,6 +236,15 @@ get_slot_component_masks_and_interp_types(struct 
>> exec_list *var_list,
>>                  get_interp_type(var, default_to_smooth_interp);
>>               interp_loc[location + i] = get_interp_loc(var);
>> +            if (var->data.always_active_io) {
>> +               /* Mark all components as used to avoid repacting xfb 
>> varyings
>> +                * wrongly. For instance, if one component of an xfb 
>> output is
>> +                * also used as input varying in the next stage.
>> +                */
>> +               comps[location + i] |= 0xf;
>> +               continue;
>> +            }
> 
> In GLSL we specifically mark the corresponding inputs as 
> always_active_io. I think we should try to do that for spirv also 
> otherwise we are going to run into issues with the other lowering passes 
> that check always_active_io such as scalar/array lowering.

https://cgit.freedesktop.org/mesa/mesa/commit/?id=a1bc1523408a305b14a8c297ea93a28bb12cee5d

Should be enough?

> 
>> +
>>               if (dual_slot) {
>>                  if (i & 1) {
>>                     comps[location + i] |= ((1 << comps_slot2) - 1);
>>
On 10/10/18 7:25 pm, Samuel Pitoiset wrote:
> On 10/10/18 10:14 AM, Timothy Arceri wrote:
>> On 10/10/18 6:44 pm, Samuel Pitoiset wrote:
>>> We shouldn't try to compact any varyings known as always
>>> active IO, especially XFB outputs. For example, if one
>>> component of an xfb output is also used as input varying
>>> in the next stage, it shouldn't be compacted.
>>>
>>> Because we look at the input varyings from the consumer
>>> stage, we don't know if one of them is an XFB output. One
>>> solution is to mark all components as used when
>>> always_active_io is true to avoid wrong remapping.
>>>
>>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
>>> ---
>>>   src/compiler/nir/nir_linking_helpers.c | 9 +++++++++
>>>   1 file changed, 9 insertions(+)
>>>
>>> diff --git a/src/compiler/nir/nir_linking_helpers.c 
>>> b/src/compiler/nir/nir_linking_helpers.c
>>> index 85712a7cb1..88014e9a1d 100644
>>> --- a/src/compiler/nir/nir_linking_helpers.c
>>> +++ b/src/compiler/nir/nir_linking_helpers.c
>>> @@ -236,6 +236,15 @@ get_slot_component_masks_and_interp_types(struct 
>>> exec_list *var_list,
>>>                  get_interp_type(var, default_to_smooth_interp);
>>>               interp_loc[location + i] = get_interp_loc(var);
>>> +            if (var->data.always_active_io) {
>>> +               /* Mark all components as used to avoid repacting xfb 
>>> varyings
>>> +                * wrongly. For instance, if one component of an xfb 
>>> output is
>>> +                * also used as input varying in the next stage.
>>> +                */
>>> +               comps[location + i] |= 0xf;
>>> +               continue;
>>> +            }
>>
>> In GLSL we specifically mark the corresponding inputs as 
>> always_active_io. I think we should try to do that for spirv also 
>> otherwise we are going to run into issues with the other lowering 
>> passes that check always_active_io such as scalar/array lowering.
> 
> https://cgit.freedesktop.org/mesa/mesa/commit/?id=a1bc1523408a305b14a8c297ea93a28bb12cee5d 
> 
> 
> Should be enough?

I haven't read the spec but if its the same as GLSL that probably only 
sets it on outputs.

> 
>>
>>> +
>>>               if (dual_slot) {
>>>                  if (i & 1) {
>>>                     comps[location + i] |= ((1 << comps_slot2) - 1);
>>>
On 10/10/18 10:34 AM, Timothy Arceri wrote:
> On 10/10/18 7:25 pm, Samuel Pitoiset wrote:
>> On 10/10/18 10:14 AM, Timothy Arceri wrote:
>>> On 10/10/18 6:44 pm, Samuel Pitoiset wrote:
>>>> We shouldn't try to compact any varyings known as always
>>>> active IO, especially XFB outputs. For example, if one
>>>> component of an xfb output is also used as input varying
>>>> in the next stage, it shouldn't be compacted.
>>>>
>>>> Because we look at the input varyings from the consumer
>>>> stage, we don't know if one of them is an XFB output. One
>>>> solution is to mark all components as used when
>>>> always_active_io is true to avoid wrong remapping.
>>>>
>>>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
>>>> ---
>>>>   src/compiler/nir/nir_linking_helpers.c | 9 +++++++++
>>>>   1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/src/compiler/nir/nir_linking_helpers.c 
>>>> b/src/compiler/nir/nir_linking_helpers.c
>>>> index 85712a7cb1..88014e9a1d 100644
>>>> --- a/src/compiler/nir/nir_linking_helpers.c
>>>> +++ b/src/compiler/nir/nir_linking_helpers.c
>>>> @@ -236,6 +236,15 @@ 
>>>> get_slot_component_masks_and_interp_types(struct exec_list *var_list,
>>>>                  get_interp_type(var, default_to_smooth_interp);
>>>>               interp_loc[location + i] = get_interp_loc(var);
>>>> +            if (var->data.always_active_io) {
>>>> +               /* Mark all components as used to avoid repacting 
>>>> xfb varyings
>>>> +                * wrongly. For instance, if one component of an xfb 
>>>> output is
>>>> +                * also used as input varying in the next stage.
>>>> +                */
>>>> +               comps[location + i] |= 0xf;
>>>> +               continue;
>>>> +            }
>>>
>>> In GLSL we specifically mark the corresponding inputs as 
>>> always_active_io. I think we should try to do that for spirv also 
>>> otherwise we are going to run into issues with the other lowering 
>>> passes that check always_active_io such as scalar/array lowering.
>>
>> https://cgit.freedesktop.org/mesa/mesa/commit/?id=a1bc1523408a305b14a8c297ea93a28bb12cee5d 
>>
>>
>> Should be enough?
> 
> I haven't read the spec but if its the same as GLSL that probably only 
> sets it on outputs.

How does the GLSL compiler handle that then? Does it somehow propagate 
always_active_io to the input varyings of the next stage?

> 
>>
>>>
>>>> +
>>>>               if (dual_slot) {
>>>>                  if (i & 1) {
>>>>                     comps[location + i] |= ((1 << comps_slot2) - 1);
>>>>