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

[Robert Bragg]

On Tue, Jan 24, 2017 at 2:37 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> 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?

oh yeah sorry, not sure how I convinced myself it took dst then src.

>
> [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.

Yup definitely, this code works for the current need so no need to
mess around with it here - thanks for clarifying my misreading.

- Robert

>
> Cheers,
>
>   -ilia