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? [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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev