On Tue, Jan 24, 2017 at 2:37 PM, Ilia Mirkin <imir...@alum.mit.edu> wrote: > On Tue, Jan 24, 2017 at 5:27 PM, Robert Bragg <rob...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev