[Mesa-dev] [PATCH] anv: implement pipeline statistics queries

[Ilia Mirkin]

On Tue, Jan 24, 2017 at 5:27 PM, Robert Bragg <robert at sixbynine.org> wrote:
>>>>>> +/*
>>>>>> + * GPR0 = GPR0 >> 2;
>>>>>> + *
>>>>>> + * Note that the upper 30 bits of GPR are lost!
>>>>>> + */
>>>>>> +static void
>>>>>> +shr_gpr0_by_2_bits(struct anv_batch *batch)
>>>>>> +{
>>>>>> +   shl_gpr0_by_30_bits(batch);
>>>>>> +   emit_load_alu_reg_reg32(batch, CS_GPR(0) + 4, CS_GPR(0));
>>>>>> +   emit_load_alu_reg_imm32(batch, CS_GPR(0) + 4, 0);
>
>
> I recently noticed from inspecting the original hsw_queryobj,c code
> that this looks suspicious.
>
> Conceptually it makes sense to implement a right shift as lshift by
> 32-n and then keeping the upper 32bits, but the emit_load_ functions
> take a destination followed by a source and so it looks like after the
> left shift it's copying the least significant 32bits of R0 over the
> most significant and then setting the most significant 32bits of R0 to
> zero. It looks like the first load_alu is redundant if the second one
> just writes zero to the same location.
>
> Maybe I'm misreading something here though, this comment it just based
> on inspection.

What you're missing, I think, is that

emit_load_alu_reg_reg32(batch, CS_GPR(0) + 4, CS_GPR(0));

does CS_GPR(0) = CS_GPR(0) + 4, and not the inverse as one logically
might have thought. I copied the semantics from the hsw_queryobj.c
file, but I think they stink. But it stinks even more to have 2
functions with inverted argument meanings.

Does that make sense?

[So we have GPR0 which is a 64-bit entity, and do GPR0 <<= 30; GPR0_LO
= GPR0_HI; GPR0_HI = 0; and then we can store GPR0 somewhere.]

As for re-using your generalized shifter, I don't think that'd make
sense to introduce in this change. It feels like a component on its
own, which should be integrated (or not) separately. When/if it is,
this and hsw_queryobj.c could migrate to using it.

Cheers,

  -ilia