On 29/08/17 21:18, Francisco Jerez wrote:
> Chema Casanova <jmcasan...@igalia.com> writes:
>
>> El 25/08/17 a las 20:09, Francisco Jerez escribió:
>>> Alejandro Piñeiro <apinhe...@igalia.com> writes:
>>>
>>>> Although it is possible to emit them directly as AND/OR on brw_fs_nir,
>>>> having specific opcodes makes it easier to remove duplicate settings
>>>> later.
>>>>
>>>> Signed-off-by: Alejandro Piñeiro <apinhe...@igalia.com>
>>>> Signed-off-by: Jose Maria Casanova Crespo <jmcasan...@igalia.com>
>>>> ---
>>>> src/intel/compiler/brw_eu.h | 3 +++
>>>> src/intel/compiler/brw_eu_defines.h | 9 +++++++++
>>>> src/intel/compiler/brw_eu_emit.c | 19 +++++++++++++++++++
>>>> src/intel/compiler/brw_fs_generator.cpp | 8 ++++++++
>>>> src/intel/compiler/brw_shader.cpp | 5 +++++
>>>> 5 files changed, 44 insertions(+)
>>>>
>>>> diff --git a/src/intel/compiler/brw_eu.h b/src/intel/compiler/brw_eu.h
>>>> index a3a9c63239d..0a7f8020398 100644
>>>> --- a/src/intel/compiler/brw_eu.h
>>>> +++ b/src/intel/compiler/brw_eu.h
>>>> @@ -500,6 +500,9 @@ brw_broadcast(struct brw_codegen *p,
>>>> struct brw_reg src,
>>>> struct brw_reg idx);
>>>>
>>>> +void
>>>> +brw_rounding_mode(struct brw_codegen *p,
>>>> + enum brw_rnd_mode mode);
>>>> /***********************************************************************
>>>> * brw_eu_util.c:
>>>> */
>>>> diff --git a/src/intel/compiler/brw_eu_defines.h
>>>> b/src/intel/compiler/brw_eu_defines.h
>>>> index 1af835d47ed..50435df2fcf 100644
>>>> --- a/src/intel/compiler/brw_eu_defines.h
>>>> +++ b/src/intel/compiler/brw_eu_defines.h
>>>> @@ -388,6 +388,9 @@ enum opcode {
>>>> SHADER_OPCODE_TYPED_SURFACE_WRITE,
>>>> SHADER_OPCODE_TYPED_SURFACE_WRITE_LOGICAL,
>>>>
>>>> + SHADER_OPCODE_RND_MODE_RTE,
>>>> + SHADER_OPCODE_RND_MODE_RTZ,
>>>> +
>>> We don't need an opcode for each possible rounding mode (there's also RU
>>> and RD). How about you add a single SHADER_OPCODE_RND_MODE opcode
>>> taking an immediate with the right rounding mode?
>> I like the proposal. It is better having a unique opcode for setting the
>> rounding mode. Changed for v2 of this patch.
>>> Also, you should be marking the rounding mode opcodes as
>>> has_side_effects(), because otherwise you're giving the scheduler the
>>> freedom of moving your rounding mode update instruction past the
>>> instruction you wanted it to have an effect on...
>> Well pointed, we already realized that it was missing while debugging
>> the latency problem of the control register. It was hiding the problem
>> re-scheduling the cr0 modification to the beginning of the shader.
>>>> SHADER_OPCODE_MEMORY_FENCE,
>>>>
>>>> SHADER_OPCODE_GEN4_SCRATCH_READ,
>>>> @@ -1233,4 +1236,10 @@ enum brw_message_target {
>>>> /* R0 */
>>>> # define GEN7_GS_PAYLOAD_INSTANCE_ID_SHIFT 27
>>>>
>>>> +enum PACKED brw_rnd_mode {
>>>> + BRW_RND_MODE_UNSPECIFIED,
>>>> + BRW_RND_MODE_RTE,
>>>> + BRW_RND_MODE_RTZ,
>>> Since you're introducing a back-end-specific rounding mode enum already,
>>> why not use the hardware values right away so you avoid hard-coding
>>> magic constants below.
>> At the end we removed MODE_UNSPECIFIED as it isn't really needed and we
>> include Round Up and Down in the enum assignation based using the PRM
>> values. Also renamed RTE for RTNE to maintain coherence with nir
>> conversion modifiers using the same acronym as PRM.
>>
>> +enum PACKED brw_rnd_mode {
>> + BRW_RND_MODE_RTNE = 0, /* Round to Nearest or Even */
>> + BRW_RND_MODE_RU = 1, /* Round Up, toward +inf */
>> + BRW_RND_MODE_RD = 2, /* Round Down, toward -inf */
>> + BRW_RND_MODE_RTZ = 3 /* Round Toward Zero */
>> +};
>>
>> I have a doubt about how to avoid the magic constants to close the v2 of
>> this patch. One approach would be using the same code structure and
>> taking advantage using the codification of rounding field. This way we
>> just formula and expect that the C compiler optimizer to guess that the
>> immediate value is a constant.
>>
>
> Hm, I'm not particularly worried about the C++ compiler behavior, it's
> most likely immaterial.
>
>> switch (mode) {
>> case BRW_RND_MODE_RTZ:
>> - inst = brw_OR(p, brw_cr0_reg(0), brw_cr0_reg(0),
>> brw_imm_ud(0x00000030u));
>> + inst = brw_OR(p, brw_cr0_reg(0), brw_cr0_reg(0),
>> brw_imm_ud(((unsigned int) mode << 4)));
>> break;
>> - case BRW_RND_MODE_RTE:
>> - inst = brw_AND(p, brw_cr0_reg(0), brw_cr0_reg(0),
>> brw_imm_ud(0xffffffcfu));
>> + case BRW_RND_MODE_RTNE:
>> + inst = brw_AND(p, brw_cr0_reg(0), brw_cr0_reg(0),
>> brw_imm_ud(((unsigned int) mode << 4) | ~0x00000030u));
>> break;
>> default:
>>
>
> I don't think this is particularly advantageous since you still need to
> special-case every possible rounding mode.
>
>> Another approach could be to implement a general solution for all
>> supported rounding modes by the hw including Round Up and Down using
>> bitwise operations.
>>
>> /**
>> * Changes the floating point rounding mode updating the control register
>> * field defined at cr0.0[5-6] bits. This function supports the changes to
>> * RTNE (00), RU (01), RD (10) and RTZ (11) rounding using bitwise
>> operations.
>> * Only RTNE and RTZ rounding are enabled at nir.
>> */
>>
>> void
>> brw_rounding_mode(struct brw_codegen *p,
>> enum brw_rnd_mode mode)
>> {
>>
>> const unsigned int mask = 0x0000000030u;
>> const unsigned int enable_bits = ((unsigned int) mode) << 4;
>> const unsigned int disable_bits = enable_bits | ~mask;
>>
>> /* Used by RTNE rounding to set field to 00 */
>> if (disable_bits != ~0) {
>> brw_AND(p, brw_cr0_reg(0), brw_cr0_reg(0), brw_imm_ud(disable_bits));
>> }
>> /* Used by RTZ rounding to set field to 11 */
>> if (enable_bits) {
>> brw_OR(p, brw_cr0_reg(0), brw_cr0_reg(0), brw_imm_ud(enable_bits));
>> }
>> }
>>
>
> Looks good (except for the SWITCH thread control flag still missing),
> though you could simplify it slightly:
Yes, Alejandro had already fixed it locally but I removed it from the to
simplify the code block.
> | void
> | brw_rounding_mode(struct brw_codegen *p, enum brw_rnd_mode mode)
> | {
> | const unsigned bits = mode << BRW_CR0_RND_MODE_SHIFT;
> |
> | if (bits != BRW_RND_MODE_MASK) {
> | brw_inst *inst = brw_AND(p, brw_cr0_reg(0), brw_cr0_reg(0),
> | brw_imm_ud(~BRW_RND_MODE_MASK));
> | brw_inst_set_thread_control(p->devinfo, inst, BRW_THREAD_SWITCH);
> | }
> |
> | if (bits) {
> | brw_inst *inst = brw_OR(p, brw_cr0_reg(0), brw_cr0_reg(0),
> | brw_imm_ud(bits));
> | brw_inst_set_thread_control(p->devinfo, inst, BRW_THREAD_SWITCH);
> | }
> | }
>
I like it and it is simpler than my proposal. I'll include the new
defines at brw_eu_defines.h including the BRW_CR0_RND_MODE_MASK and I'll
submit v2.
Thanks
Chema
>
>> Any preference ?
>>
>> Thanks for the review.
>>
>> Chema
>>
>>>
>>>> +};
>>>> +
>>>> #endif /* BRW_EU_DEFINES_H */
>>>> diff --git a/src/intel/compiler/brw_eu_emit.c
>>>> b/src/intel/compiler/brw_eu_emit.c
>>>> index 0b0d67a5c56..07ad3d9384b 100644
>>>> --- a/src/intel/compiler/brw_eu_emit.c
>>>> +++ b/src/intel/compiler/brw_eu_emit.c
>>>> @@ -3723,3 +3723,22 @@ brw_WAIT(struct brw_codegen *p)
>>>> brw_inst_set_exec_size(devinfo, insn, BRW_EXECUTE_1);
>>>> brw_inst_set_mask_control(devinfo, insn, BRW_MASK_DISABLE);
>>>> }
>>>> +
>>>> +void
>>>> +brw_rounding_mode(struct brw_codegen *p,
>>>> + enum brw_rnd_mode mode)
>>>> +{
>>>> + switch (mode) {
>>>> + case BRW_RND_MODE_UNSPECIFIED:
>>>> + /* nothing to do here */
>>>> + break;
>>>> + case BRW_RND_MODE_RTZ:
>>>> + brw_OR(p, brw_cr0_reg(0), brw_cr0_reg(0), brw_imm_ud(0x00000030u));
>>>> + break;
>>>> + case BRW_RND_MODE_RTE:
>>>> + brw_AND(p, brw_cr0_reg(0), brw_cr0_reg(0), brw_imm_ud(0xffffffcfu));
>>> This has undefined behavior because the ALU instructions you use to set
>>> cr0 have non-zero latency, so the rounding mode change won't take effect
>>> till ~8 cycles after the instruction is issued. Any instructions issued
>>> in that window will pick up the wrong rounding mode. This is likely one
>>> of the reasons for your observations off-list regarding some shaders
>>> using the right or wrong rounding mode non-deterministically depending
>>> on the scheduler's behaviour.
>>>
>>> Here's a spec quote from the SKL PRM suggesting a workaround you should
>>> probably include in this commit:
>>>
>>> | Implementation Restriction on Register Access: When the control
>>> | register is used as an explicit source and/or destination, hardware
>>> | does not ensure execution pipeline coherency. Software must set the
>>> | thread control field to ‘switch’ for an instruction that uses control
>>> | register as an explicit operand. This is important as the control
>>> | register is an implicit source for most instructions. For example,
>>> | fields like FPMode and Accumulator Disable control the arithmetic
>>> | and/or logic instructions. Therefore, if the instruction updating the
>>> | control register doesn’t set ‘switch’, subsequent instructions may
>>> | have undefined results.
>>>
>>>
>>>> + break;
>>>> + default:
>>>> + unreachable("Not reached");
>>>> + }
>>>> +}
>>>> diff --git a/src/intel/compiler/brw_fs_generator.cpp
>>>> b/src/intel/compiler/brw_fs_generator.cpp
>>>> index 2ade486705b..e0bd191ea7e 100644
>>>> --- a/src/intel/compiler/brw_fs_generator.cpp
>>>> +++ b/src/intel/compiler/brw_fs_generator.cpp
>>>> @@ -2139,6 +2139,14 @@ fs_generator::generate_code(const cfg_t *cfg, int
>>>> dispatch_width)
>>>> brw_DIM(p, dst, retype(src[0], BRW_REGISTER_TYPE_F));
>>>> break;
>>>>
>>>> + case SHADER_OPCODE_RND_MODE_RTE:
>>>> + brw_rounding_mode(p, BRW_RND_MODE_RTE);
>>>> + break;
>>>> +
>>>> + case SHADER_OPCODE_RND_MODE_RTZ:
>>>> + brw_rounding_mode(p, BRW_RND_MODE_RTZ);
>>>> + break;
>>>> +
>>>> default:
>>>> unreachable("Unsupported opcode");
>>>>
>>>> diff --git a/src/intel/compiler/brw_shader.cpp
>>>> b/src/intel/compiler/brw_shader.cpp
>>>> index c62b8ba6140..f22e204e262 100644
>>>> --- a/src/intel/compiler/brw_shader.cpp
>>>> +++ b/src/intel/compiler/brw_shader.cpp
>>>> @@ -486,6 +486,11 @@ brw_instruction_name(const struct gen_device_info
>>>> *devinfo, enum opcode op)
>>>> return "tes_add_indirect_urb_offset";
>>>> case TES_OPCODE_GET_PRIMITIVE_ID:
>>>> return "tes_get_primitive_id";
>>>> +
>>>> + case SHADER_OPCODE_RND_MODE_RTE:
>>>> + return "round_mode_rte";
>>>> + case SHADER_OPCODE_RND_MODE_RTZ:
>>>> + return "round_mode_rtz";
>>>> }
>>>>
>>>> unreachable("not reached");
>>>> --
>>>> 2.11.0
>>>>
>>>> _______________________________________________
>>>> mesa-dev mailing list
>>>> mesa-dev@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>>>
>>>>
>>>> _______________________________________________
>>>> mesa-dev mailing list
>>>> mesa-dev@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
