[Mesa-dev,17/32] i965/vec4: Fix constant propagation across different types.

Submitted by Francisco Jerez on Feb. 6, 2015, 2:42 p.m.

Details

Message ID 1423233792-11767-17-git-send-email-currojerez@riseup.net
State New
Headers show

Not browsing as part of any series.

Commit Message

Francisco Jerez Feb. 6, 2015, 2:42 p.m.
If the source type differs from the original type of the constant we
need to bit-cast it before propagating, otherwise the original type
information will be lost.  If the constant was a vector float there
isn't much we can do, because the result of bit-casting the component
values of a vector float cannot itself be represented as an immediate.
---
 src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp | 10 ++++++++++
 1 file changed, 10 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
index 81567d2..0a961ce 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
+++ b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
@@ -113,6 +113,16 @@  try_constant_propagate(struct brw_context *brw, vec4_instruction *inst,
    if (value.file != IMM)
       return false;
 
+   if (value.type == BRW_REGISTER_TYPE_VF) {
+      /* The result of bit-casting the component values of a vector float
+       * cannot in general be represented as an immediate.
+       */
+      if (inst->src[arg].type != BRW_REGISTER_TYPE_F)
+         return false;
+   } else {
+      value.type = inst->src[arg].type;
+   }
+
    if (inst->src[arg].abs) {
       if (!brw_abs_immediate(value.type, &value.fixed_hw_reg)) {
          return false;

Comments

On Fri, Feb 6, 2015 at 6:42 AM, Francisco Jerez <currojerez@riseup.net> wrote:
> If the source type differs from the original type of the constant we
> need to bit-cast it before propagating, otherwise the original type
> information will be lost.  If the constant was a vector float there
> isn't much we can do, because the result of bit-casting the component
> values of a vector float cannot itself be represented as an immediate.
> ---
>  src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
> index 81567d2..0a961ce 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
> @@ -113,6 +113,16 @@ try_constant_propagate(struct brw_context *brw, vec4_instruction *inst,
>     if (value.file != IMM)
>        return false;
>
> +   if (value.type == BRW_REGISTER_TYPE_VF) {
> +      /* The result of bit-casting the component values of a vector float
> +       * cannot in general be represented as an immediate.
> +       */
> +      if (inst->src[arg].type != BRW_REGISTER_TYPE_F)
> +         return false;
> +   } else {
> +      value.type = inst->src[arg].type;
> +   }
> +
>     if (inst->src[arg].abs) {
>        if (!brw_abs_immediate(value.type, &value.fixed_hw_reg)) {
>           return false;

Wow. Did you hit this in practice?

Actually, I suspect this might be the cause of
https://bugs.freedesktop.org/show_bug.cgi?id=86811 (only reacquired a
BDW recently, so never investigated)

Hm, but running the test with INTEL_DEVID_OVERRIDE and
INTEL_DEBUG=vec4vs doesn't show the VF immediate being propagated into
the shift, so perhaps it's not the problem.
Matt Turner <mattst88@gmail.com> writes:

> On Fri, Feb 6, 2015 at 6:42 AM, Francisco Jerez <currojerez@riseup.net> wrote:
>> If the source type differs from the original type of the constant we
>> need to bit-cast it before propagating, otherwise the original type
>> information will be lost.  If the constant was a vector float there
>> isn't much we can do, because the result of bit-casting the component
>> values of a vector float cannot itself be represented as an immediate.
>> ---
>>  src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
>> index 81567d2..0a961ce 100644
>> --- a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
>> @@ -113,6 +113,16 @@ try_constant_propagate(struct brw_context *brw, vec4_instruction *inst,
>>     if (value.file != IMM)
>>        return false;
>>
>> +   if (value.type == BRW_REGISTER_TYPE_VF) {
>> +      /* The result of bit-casting the component values of a vector float
>> +       * cannot in general be represented as an immediate.
>> +       */
>> +      if (inst->src[arg].type != BRW_REGISTER_TYPE_F)
>> +         return false;
>> +   } else {
>> +      value.type = inst->src[arg].type;
>> +   }
>> +
>>     if (inst->src[arg].abs) {
>>        if (!brw_abs_immediate(value.type, &value.fixed_hw_reg)) {
>>           return false;
>
> Wow. Did you hit this in practice?
>
I hit the non-VF case in practice (constant propagation forgetting the
original type of the source when propagating a bit-cast constant of a
different type).  I haven't seen the VF case in the wild but my addition
wouldn't have seemed correct without taking vector immediates into
account somehow.

> Actually, I suspect this might be the cause of
> https://bugs.freedesktop.org/show_bug.cgi?id=86811 (only reacquired a
> BDW recently, so never investigated)
>
> Hm, but running the test with INTEL_DEVID_OVERRIDE and
> INTEL_DEBUG=vec4vs doesn't show the VF immediate being propagated into
> the shift, so perhaps it's not the problem.

Hm, yes, I guess that might be something else...
On Fri, Feb 6, 2015 at 4:17 PM, Francisco Jerez <currojerez@riseup.net> wrote:
> Matt Turner <mattst88@gmail.com> writes:
>
>> On Fri, Feb 6, 2015 at 6:42 AM, Francisco Jerez <currojerez@riseup.net> wrote:
>>> If the source type differs from the original type of the constant we
>>> need to bit-cast it before propagating, otherwise the original type
>>> information will be lost.  If the constant was a vector float there
>>> isn't much we can do, because the result of bit-casting the component
>>> values of a vector float cannot itself be represented as an immediate.
>>> ---
>>>  src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
>>> index 81567d2..0a961ce 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
>>> +++ b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
>>> @@ -113,6 +113,16 @@ try_constant_propagate(struct brw_context *brw, vec4_instruction *inst,
>>>     if (value.file != IMM)
>>>        return false;
>>>
>>> +   if (value.type == BRW_REGISTER_TYPE_VF) {
>>> +      /* The result of bit-casting the component values of a vector float
>>> +       * cannot in general be represented as an immediate.
>>> +       */
>>> +      if (inst->src[arg].type != BRW_REGISTER_TYPE_F)
>>> +         return false;
>>> +   } else {
>>> +      value.type = inst->src[arg].type;
>>> +   }
>>> +
>>>     if (inst->src[arg].abs) {
>>>        if (!brw_abs_immediate(value.type, &value.fixed_hw_reg)) {
>>>           return false;
>>
>> Wow. Did you hit this in practice?
>>
> I hit the non-VF case in practice (constant propagation forgetting the
> original type of the source when propagating a bit-cast constant of a
> different type).  I haven't seen the VF case in the wild but my addition
> wouldn't have seemed correct without taking vector immediates into
> account somehow.

Okay, I really do need to see an example. is_direct_copy() checks that
the types are suitable already.
Matt Turner <mattst88@gmail.com> writes:

> On Fri, Feb 6, 2015 at 4:17 PM, Francisco Jerez <currojerez@riseup.net> wrote:
>> Matt Turner <mattst88@gmail.com> writes:
>>
>>> On Fri, Feb 6, 2015 at 6:42 AM, Francisco Jerez <currojerez@riseup.net> wrote:
>>>> If the source type differs from the original type of the constant we
>>>> need to bit-cast it before propagating, otherwise the original type
>>>> information will be lost.  If the constant was a vector float there
>>>> isn't much we can do, because the result of bit-casting the component
>>>> values of a vector float cannot itself be represented as an immediate.
>>>> ---
>>>>  src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp | 10 ++++++++++
>>>>  1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
>>>> index 81567d2..0a961ce 100644
>>>> --- a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
>>>> +++ b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
>>>> @@ -113,6 +113,16 @@ try_constant_propagate(struct brw_context *brw, vec4_instruction *inst,
>>>>     if (value.file != IMM)
>>>>        return false;
>>>>
>>>> +   if (value.type == BRW_REGISTER_TYPE_VF) {
>>>> +      /* The result of bit-casting the component values of a vector float
>>>> +       * cannot in general be represented as an immediate.
>>>> +       */
>>>> +      if (inst->src[arg].type != BRW_REGISTER_TYPE_F)
>>>> +         return false;
>>>> +   } else {
>>>> +      value.type = inst->src[arg].type;
>>>> +   }
>>>> +
>>>>     if (inst->src[arg].abs) {
>>>>        if (!brw_abs_immediate(value.type, &value.fixed_hw_reg)) {
>>>>           return false;
>>>
>>> Wow. Did you hit this in practice?
>>>
>> I hit the non-VF case in practice (constant propagation forgetting the
>> original type of the source when propagating a bit-cast constant of a
>> different type).  I haven't seen the VF case in the wild but my addition
>> wouldn't have seemed correct without taking vector immediates into
>> account somehow.
>
> Okay, I really do need to see an example. is_direct_copy() checks that
> the types are suitable already.

*Shrug*, is_direct_copy() doesn't help because it doesn't have the
chance to check the types of the instruction a copy is being propagated
into.  For instance,

| MOV g1.0:d 0xdeadbeef:d
| ADD g2.0:f g2.0:f g1.0:f

currently gets constant-propagated into:

| ADD g2.0:f g2.0:f 0xdeadbeef:d

which is not what the original program means.