[Beignet] [PATCH 2/4] support sends (split send) for untyped write

[Guo, Yejun]

please hold on my v2 patch.

let me first try to refine code starting from GenEncoder::setMessageDescriptor. 

-----Original Message-----
From: Song, Ruiling 
Sent: Thursday, November 24, 2016 8:15 PM
To: Guo, Yejun; beignet at lists.freedesktop.org
Subject: RE: [Beignet] [PATCH 2/4] support sends (split send) for untyped write



> -----Original Message-----
> From: Guo, Yejun
> Sent: Thursday, November 24, 2016 10:55 AM
> To: Song, Ruiling <ruiling.song at intel.com>; 
> beignet at lists.freedesktop.org
> Subject: RE: [Beignet] [PATCH 2/4] support sends (split send) for 
> untyped write
> 
> > +  unsigned
> > Gen9Encoder::setUntypedWriteSendsMessageDesc(GenNativeInstruction
> > *insn, unsigned bti, unsigned elemNum)
> The message desc encoding is same for send and sends, what about 
> calling existing function?
> 66   void Gen8Encoder::setDPUntypedRW(GenNativeInstruction *insn,
>  67                                     uint32_t bti,
>  68                                     uint32_t rgba,
>  69                                     uint32_t msg_type,
>  70                                     uint32_t msg_length,
>  71                                     uint32_t response_length)
> [yejun] there is a change, 
> gen9_insn->bits3.sends_untyped_rw.src0_length is 1 for SIMD8 and 2 for SIM16, not the msg_length.
>                I don't want to reuse the Gen8Encoder function with 
> msg_length=1 since it is different at concept level.
If you think it in another perspective, message descriptor describe src0_length for both send and sends.
They are unified at concept level.
>               Another reason is that setDPUntypedRW calls 
> setMessageDescriptor which changes other bits, it is not a pure function to set the msg desc.
I agree with you the setDPUntypedRW is a bad design. But the correct way to do is refine the function and reuse it.
Just moving the sfid setting out of the function. Then things will be clearer.
in the setMessageDescriptor() it calls setSrc1(). This should be also moved out.
> 
> 
> > +      gen9_insn->bits1.sends.dest_reg_file_0 = 1;    //01 for GRF
> Generally, we should set sends destination to null register, so it is ARF.
> [yejun] I'm open to use GRF or ARF, but does this bit really matter, 
> is there a description mentions null register is ARF in hw spec?
Check the chapter architecture register file.
> 
> 
> > +      gen9_insn->bits2.sends.src0_subreg_nr = addr.subnr;
> Setting src0_subreg_nr here is meaningless, only the src0_subreg_nr[4] 
> bit left, I am not sure whether hw use it correctly.
> Generally the message payload register subnr should be 0. You can 
> remove above line, add an assert(addr.subnr == 0); [Yejun] Actually, 
> there are subreg_nr for src0 and dst, I'm not sure if 
> src0_subreg_nr[4] is a typo in spec. Anyway, add an assert will be a 
> good choice for unclear things.
Src0_subreg_nr[3:0] bits that it bits[67:64] was already taken by ExDesc[9:6].
So you cannot set src0_subreg_nr like above.
> 
> And I would also suggest you define below functions to implement sends 
> encoding logic as sends has very different encoding.
> setSendsDst(nullreg);
> setSendsSrc0(src0);
> setSendsSrc1(src1);
> so that untyped_write() byte_scatter() typed_write can call these 
> functions instead of repeating same logic at every place.
> [Yejun] Actually, I'm refining the code locally with 
> setSendsOperand(dst, src0, src1). What's your opinion?
Separate would be better. I noticed you already sent new version, setting all operands together seems acceptable.