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

[Ilia Mirkin]

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.

Again, what problem was this patch trying to solve?

  -ilia