[Mesa-dev] [PATCH] i965/skl: Add the header for constant loads outside of the generator

[Neil Roberts]

Ben Widawsky <ben at bwidawsk.net> writes:

>> +   void generate_set_simd4x2_header_gen9(vec4_instruction *inst,
>> +                                         struct brw_reg dst);
>
> super bikesheddy, but this function name sounds pretty bad, though I
> understand it's a side-effect of the name of the opcode (which seems
> consistent with similar opcodes). I can't think of anything great
> either though.

Yes, it's a bit awkward, but unless someone proposes a better name I
will leave it. It matches methods like
fs_generator::generate_set_simd4x2_offset which already exists.

>>  void
>> +vec4_generator::generate_set_simd4x2_header_gen9(vec4_instruction *inst,
>> +                                                 struct brw_reg dst)
>> +{
>> +   /* Skylake requires a message header in order to use SIMD4x2 mode. */
>
> This comment is pretty useless here.

Yes, good point. I'll remove it.

>> +   brw_push_insn_state(p);
>> +   brw_set_default_mask_control(p, BRW_MASK_DISABLE);
>> +
>> +   brw_MOV(p, dst, retype(brw_vec4_grf(0, 0), BRW_REGISTER_TYPE_UD));
>> +
>
> retype(brw_vec8_grf(0, 0)? It would jive better with the diff.

Ah yes, oops. I've accidentally undone the fix I made in 07c571a39fa1
here. I'll fix it.

>> +void
>> +vec4_visitor::emit_pull_constant_load_reg(dst_reg dst,
>> +                                          src_reg surf_index,
>> +                                          src_reg offset_reg,
>> +                                          bblock_t *before_block,
>> +                                          vec4_instruction *before_inst)
>> +{
>> +   assert((before_inst == NULL && before_block == NULL) ||
>> +          (before_inst && before_block));
>> +
>> +   vec4_instruction *pull;
>> +
>> +   if (brw->gen >= 9) {
>> +      /* Gen9+ needs a message header in order to use SIMD4x2 mode */
>> +      src_reg header(this, glsl_type::uvec4_type, 2);
>> +
>
> Just curious why you didn't go for a uvec8_type. It seems more natural
> for what we want to do, and how the other code handles things.

I don't think I picked uvec4_type here for any particular reason. I
guess it's hard to pick a type that makes sense because it's a virtual
register that needs to be sized for 2 actual registers where the first
one is a chunk of structed data used for the header and the second one
is actually just an integer I think. As Ian pointed out, uvec8_type
doesn't exist.

>> +      dst_reg index_reg = retype(offset(dst_reg(header), 1),
>> +                                 offset_reg.type);
>> +      pull = MOV(writemask(index_reg, WRITEMASK_X), offset_reg);
>
> I am not finding where the 1 in the offset there comes from? I guess
> that's why you used a uvec4 above, though I think suboffset can
> achieve the same. Can you elaborate for me, I was sort of expecting to
> see a '2' instead of 1?

This isn't a suboffset into a register, the 1 means to add a whole
register. I think the register type is irrelevant in that case. The
header register is allocated as two-registers wide. The first actual
register gets the header and here we are copying in the index into the
second register.

> I wouldn't be opposed to doing the extraction as a prep-patch fwiw.
> Should make things slightly easier to review, but I can live with
> whatever you prefer.

Ok, good idea. I'll do that.

> I couldn't spot any issues otherwise. If you can address my questions
> comments, I think I'm ready to slap on the r-b

Thanks for the review.

Regards,
- Neil