[Mesa-stable] [Mesa-dev] [PATCH 1/2] gm107/ir: make use of FADD32I for all immediates

[Samuel Pitoiset]

On 06/28/2016 04:15 PM, Ilia Mirkin wrote:
> On Tue, Jun 28, 2016 at 10:11 AM, Samuel Pitoiset
> <samuel.pitoiset at gmail.com> wrote:
>>
>>
>> On 06/28/2016 04:00 PM, Ilia Mirkin wrote:
>>>
>>> On Tue, Jun 28, 2016 at 4:33 AM, Samuel Pitoiset
>>> <samuel.pitoiset at gmail.com> wrote:
>>>>
>>>>
>>>>
>>>> On 06/28/2016 05:10 AM, Ilia Mirkin wrote:
>>>>>
>>>>>
>>>>> On Mon, Jun 27, 2016 at 6:08 PM, Samuel Pitoiset
>>>>> <samuel.pitoiset at gmail.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 06/28/2016 12:06 AM, Ilia Mirkin wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Mon, Jun 27, 2016 at 6:05 PM, Ilia Mirkin <imirkin at alum.mit.edu>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Mon, Jun 27, 2016 at 6:04 PM, Samuel Pitoiset
>>>>>>>> <samuel.pitoiset at gmail.com> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 06/28/2016 12:02 AM, Ilia Mirkin wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> This loses you saturation. Does the target account for this?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> No saturate flag for FADD32I.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> That's not what I asked.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Specifically look at this code:
>>>>>>>
>>>>>>> bool
>>>>>>> TargetNVC0::isSatSupported(const Instruction *insn) const
>>>>>>> {
>>>>>>>    if (insn->op == OP_CVT)
>>>>>>>       return true;
>>>>>>>    if (!(opInfo[insn->op].dstMods & NV50_IR_MOD_SAT))
>>>>>>>       return false;
>>>>>>>
>>>>>>>    if (insn->dType == TYPE_U32)
>>>>>>>       return (insn->op == OP_ADD) || (insn->op == OP_MAD);
>>>>>>>
>>>>>>>    // add f32 LIMM cannot saturate
>>>>>>>    if (insn->op == OP_ADD && insn->sType == TYPE_F32) {
>>>>>>>       if (insn->getSrc(1)->asImm() &&
>>>>>>>           insn->getSrc(1)->reg.data.u32 & 0xfff)
>>>>>>>          return false;
>>>>>>>    }
>>>>>>>
>>>>>>> Note how it will say that sat is supported for SIMMs with FADD? So the
>>>>>>> compiler will generate those ops, but then the emitter won't be able
>>>>>>> to handle it.
>>>>>>>
>>>>>>
>>>>>> Okay, I get it.
>>>>>
>>>>>
>>>>>
>>>>> By the way, instead of trying to fight the longIMMD, you should just fix
>>>>> it -
>>>>>
>>>>>         /*0008*/               @P0 FADD R0, R0, 1.NEG;      /*
>>>>> 0x3858203f80000000 */
>>>>>
>>>>> which corresponds nicely to
>>>>>
>>>>>       emitNEG(0x2d, insn->src(1));
>>>>>
>>>>> The issue is that emitIMMD does
>>>>>
>>>>>    if (len == 19) {
>>>>> ...
>>>>>       emitField( 56,   1, (val & 0x80000) >> 19);
>>>>>       emitField(pos, len, (val & 0x7ffff));
>>>>>
>>>>> So the problem is that the 56 isn't as fixed as the emission code had
>>>>> hoped. I suspect that adjusting it will fix all these silly cases.
>>>>>
>>>>>   -ilia
>>>>>
>>>>
>>>>         /*0010*/               @P0 FADD R0, R0, 0.NEG;      /*
>>>> 0x3858200000000000 */
>>>>         /*0010*/               @P0 FADD R0, R0, -0;      /*
>>>> 0x3958000000000000 */
>>>>
>>>> urgh?
>>>
>>>
>>> So ... what problem were you having again?
>>
>>
>> The thing is: why those 2 instructions use a different position for the neg
>> flag?
>
> One is setting the high bit of the immediate, the other is applying
> negation to the argument.

Ok.

>
> Again, what problem was this patch trying to solve?

The problem is that FADD can only emits 19-bits but longIMMD() will 
return false because it only checks for the high 12-bits.

I don't know if you saw my messages on IRC but I found some other issues 
with longIMMD() and emitIMMD().

>
>   -ilia
>

-- 
-Samuel