[Mesa-dev] [PATCH v2 000/103] i965 Haswell ARB_gpu_shader_fp64 / OpenGL 4.0

[Iago Toral]

On Tue, 2016-12-13 at 13:22 +0100, Samuel Iglesias Gonsálvez wrote:
> On Sun, 2016-12-11 at 15:00 -0800, Matt Turner wrote:
> > 
> > i965/vec4: handle 32 and 64 bit channels in liveness analysis
> > 
> > 	Please indent the returned multiline expressions in
> > 	var_from_reg() like we do elsewhere, so that the second line
> > 	begins on the same column as the first line.
> > 
> > 	*/ goes on its own line.
> > 
> > 	I'm having a hard time reviewing this one. The logic is rather
> > 	complex. I'll ask someone to help me review it on Monday at the
> > 	office.
> > 
> OK, no problem!

I think for this one you want to ping Curro, he reviewed parts of it
when Juan wrote it and he suggested changes so he is familiar with the
solution.

Iago

> > 
> > i965/vec4: add a horiz_offset() helper
> > i965: move the group field from fs_inst to backend_instruction.
> > i965/vec4: add a SIMD lowering pass
> > 
> > 	In the commit message, you say
> > 
> > 	    For now the pass only handles the gen7 restriction where
> > any
> > 	    instruction that writes 2 registers also needs to read 2
> > 	    registers.  This affects double-precision instructions
> > 	    reading uniforms, for example. Later patches will extend
> > the
> > 	    lowering pass adding a few more cases.
> > 
> > 	But the rule about if-writing-two-regs, must-read-two-regs
> > 	says that scalar sources are an exception:
> > 
> > 		"When source is scalar, the source registers are not
> > 		 incremented."
> > 
> > 	I don't see any code that allows us to avoid splitting an
> > 	instruction if it's writing two registers but sourcing a scalar
> > 	uniform. Maybe this doesn't apply because we have to use a non
> > 	scalar swizzle (.xy) to access a single fp64 component?
> > 
> Right, however, this is necessary in align16 because uniforms are
> vectors (hstride != 0) so they are not scalars.
> 
> > 
> > i965/vec4: make the generator set correct NibCtrl for SIMD4 DF
> > instructions
> > i965/vec4: dump NibCtrl for instructions with execsize != 8
> > i965/disasm: print NibCtrl for instructions with execsize < 8
> > i965/vec4: teach CSE about exec_size, group and doubles
> > i965/vec4: teach cmod propagation about different execution sizes
> > i965/vec4: split double-precision bcsel
> > 
> > 	bcsel is the NIR opcode. I'd change references to bcsel to SEL.
> > 
> > 	Very interesting find...
> > 
> > i965/vec4: add a scalarization pass for double-precision
> > instructions
> > 
> > 	Don't indent case inside a switch.
> > 
> > i965/vec4: translate 64-bit swizzles to 32-bit
> > i965/vec4: implement access to DF source components Z/W
> > 
> > 	Wow, bien hecho!
> > 
> > i965/disasm: fix subreg for dst in Align16 mode
> > i965/vec4: teach register coalescing about 64-bit
> > i965/vec4: fix pack_uniform_registers for doubles
> > i965/vec4: fix indentation in pack_uniform_registers
> > i965/vec4: Skip swizzle to subnr in 3src instructions with DF
> > operands
> > 
> > 	s/need/needs/ in the comment.
> > 
> > i965/vec4/nir: do not emit 64-bit MAD
> > i965/vec4: do not emit 64-bit MAD
> > 
> > 	I might change the name of this commit to "i965/vec4: Lower
> > 	64-bit MAD" or "i965/vec4: Lower DF MAD"
> > 
> > 	I think I'd change the name of the function as well, maybe to
> > 	lower_64bit_mad[_to_mul_add] or something.
> > 
> OK, we will do the rename.
> 
> > 
> > i965/vec4: support multiple dispatch widths and groups in the IR
> > builder.
> > i965/vec4: Add a shuffle_64bit_data helper
> > 
> > 	I was initially confused by r0.0:DF/r0.1:DF, thinking that .1
> > in
> > 	r0.1:DF was a subreg offset. But I think it's actually the
> > 	register offset (i.e., .offset)?
> > 
> > 	If that's the case, I think it would be clearer just to
> > 	increment the register number in the example:
> > 
> > 		r0.0:DF  x0 y0 z0 w0
> > 		r1.0:DF  x1 y1 z1 w1
> > 
> > 	s/opperation/operation/ in the comment.
> > 
> > 	On the multiline bld.group(...), I think Curro's style is to
> > 	align with the '.'. For instance,
> > 
> > 	inst = bld.group(4, for_write ? 1 : 0)
> > 	          .MOV(writemask(dst, WRITEMASK_ZW),
> > 	               swizzle(byte_offset(src, REG_SIZE),
> > BRW_SWIZZLE_XYXY));
> > 
> > 	so that group and MOV align, with the '.' on the same line as
> > 	the MOV.
> > 
> Regarding the example, yes, you are right. We are going to fix it.
> Thanks for the rest of suggestions, we will do them too :-)
> 
> 
> > 
> > i965/vec4: Fix UBO loads for 64-bit data
> > i965/vec4: Fix SSBO loads for 64-bit data
> > i965/vec4: Fix SSBO stores for 64-bit data
> > i965/vec4: don't constant propagate 64-bit immediates
> > i965/vec4: prevent copy-propagation from values with a different
> > type
> > size
> > i965/vec4: Prevent copy propagation from violating pre-gen8
> > restrictions
> > 
> > 	Similar comment as before about being allowed to write two
> > 	registers while sourcing a scalar. Maybe doesn't apply because
> > 	of the double swizzle.
> > 
> Same reply as to the other question.
> 
> > 
> > i965/vec4: don't propagate single-precision uniforms into 4-wide
> > instructions
> > i965/vec4: don't copy propagate misaligned registers
> > i965/vec4: extend the DWORD multiply DepCtrl restriction to all
> > gen8
> > platforms
> > 
> > 	I don't see this in the BDW PRMs, and the internal
> > documentation
> > 	says that it applies to "CHV, BXT"
> > 
> > 	I suggest dropping this patch (or replacing it with one that
> > 	adds || devinfo->is_broxton).
> > 
> It is present in BDW PRMs, Volume 7: 3D-Media-GPGPU, "Special
> Requirements for Handling Double Precision Data Types",
> page 837:
>     
> "When source or destination datatype is 64b or operation is integer
> DWord multiply, DepCtrl must not be used."
> 
> Regarding Broxton, there is no public PRMs here [0]. So if your
> internal docs say it is needed, then we must fix this patch.
> 
> [0] https://01.org/linuxgraphics/documentation/hardware-specification
> -p
> rms
> 
> 
> > 
> > i965/vec4: Do not use DepCtrl with 64-bit instructions
> > i965/vec4: do not split scratch read/write opcodes
> > i965/vec4: fix scratch offset for 64bit data
> > i965/vec4: fix scratch reads for 64bit data
> > i965/vec4: fix scratch writes for 64bit data
> > i965/vec4: fix move_uniform_array_access_to_pull_constant() for 64-
> > bit data
> > i965/vec4: fix indentation in
> > move_push_constants_to_pull_constants()
> > i965/vec4: fix move_push_constants_to_pull_constants() for 64-bit
> > data
> > i965/vec4: make emit_pull_constant_load support 64-bit loads
> > i965/vec4: fix indentation in lower_attributes_to_hw_regs()
> > i965/vec4: fix attribute setup for doubles
> > i965/vec4: fix store output for 64-bit types
> > i965/vec4/gs: fix input loading for 64bit data
> > i965/vec4/tcs: fix input loading for 64-bit data
> > i965/vec4/tcs: fix outputs for 64-bit data
> > i965/vec4/tes: fix input loading for 64bit data types
> > i965/vec4/tes: fix setup_payload() for 64bit data types
> > i965/vec4/tes: consider register offsets during attribute setup
> > i965/vec4: dump subnr for FIXED_GRF
> > i965/vec4: split instructions that read 64-bit interleaved
> > attributes
> > 
> > 	I think you want to make stage_uses_interleaved_attributes
> > 	static.
> > 
> > 	Also don't indent case inside switch.
> > 
> > i965/vec4/scalarize_df: do not scalarize swizzles that we can
> > support
> > natively
> > i965/vec4/scalarize_df: support more swizzles via vstride=0
> > i965/vec4: prevent src/dst hazards during 64-bit register
> > allocation
> > i965/vec4: run scalarize_df() after spilling
> > 
> > 	Calling scalarize_df() on code that's already been scalarized
> > 	seems a little scary...
> > 
> > 	Are you sure that scalarize_df() will not modify any code that
> > 	has already been scalarized?
> > 
> Yes, it is safe. The scalarization pass operates on logical
> channels, 
> so for something like:
> 
> 		mov r0.xy:df r1.zw:df
> 
> it produces:
> 
> 		mov r0.x:df r1.z:dfmov r0.y:df r1.w:df
> 
> and if we run the pass again nothing will change for these
> instructions
> because they only operate on a single channel, so we end up with the
> same thing. It would have been a different story if we also
> translated
> the logical channels into 32-bit channels during the scalarization
> process, but that won't happen until right before codegen precisely
> so
> we can avoid this sort of problems (specifically we do it in
> convert_to_hw_regs).
> 
> > 
> > i965/vec4: support basic spilling of 64-bit registers
> > i965/vec4: avoid spilling of registers that mix 32-bit and 64-bit
> > access
> > i965/vec4: prevent spilling of DOUBLE_TO_SINGLE destination
> > i965/vec4: adjust spilling costs for 64-bit registers.
> > i965/vec4: enable ARB_gpu_shader_fp64 for Haswell
> > i965/gen7: expose OpenGL 4.0 on Haswell
> > 
> > 	There's been a lot of discussion about this patch that I
> > haven't
> > 	been involved in. Presumably given my review of the previous
> > 100
> > 	patches, someone previously involved can be bothered to
> > rereview
> > 	this one...
> OK, no problem. Thanks for reviewing all the other patches! We will
> ping Curro about this one since he was the one who triggered the v3.
> 
> Sam