Re: [Mesa-dev] [PATCH] intel/compiler: Reset default flag register in brw_find_live_channel()
On Tue, Jan 22, 2019 at 3:25 PM Francisco Jerez wrote: > > Matt Turner writes: > > > emit_uniformize() emits SHADER_OPCODE_FIND_LIVE_CHANNEL with its > > flag_subreg set, so that the IR knows which flag is accessed. However > > the flag is only used on Gen7 in Align1 mode. > > > > To avoid setting unnecessary bits in the instruction words, get the > > information we need and reset the default flag register. This allows > > round-tripping through the assembler/disassembler. > > --- > > src/intel/compiler/brw_eu_emit.c | 12 ++-- > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/src/intel/compiler/brw_eu_emit.c > > b/src/intel/compiler/brw_eu_emit.c > > index 45e2552783b..7c5b40af3ae 100644 > > --- a/src/intel/compiler/brw_eu_emit.c > > +++ b/src/intel/compiler/brw_eu_emit.c > > @@ -3312,6 +3312,13 @@ brw_find_live_channel(struct brw_codegen *p, struct > > brw_reg dst, > > > > brw_push_insn_state(p); > > > > + /* The flag register is only used on Gen7 in align1 mode, so avoid > > setting > > +* unnecessary bits in the instruction words, get the information we > > need > > +* and reset the default flag register. > > Maybe mention here that this also allows more instructions to be > compacted. Looks good otherwise: > > Reviewed-by: Francisco Jerez Sure, will do. Thanks Curro! ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] intel/compiler: Reset default flag register in brw_find_live_channel()
Matt Turner writes:
> emit_uniformize() emits SHADER_OPCODE_FIND_LIVE_CHANNEL with its
> flag_subreg set, so that the IR knows which flag is accessed. However
> the flag is only used on Gen7 in Align1 mode.
>
> To avoid setting unnecessary bits in the instruction words, get the
> information we need and reset the default flag register. This allows
> round-tripping through the assembler/disassembler.
> ---
> src/intel/compiler/brw_eu_emit.c | 12 ++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/src/intel/compiler/brw_eu_emit.c
> b/src/intel/compiler/brw_eu_emit.c
> index 45e2552783b..7c5b40af3ae 100644
> --- a/src/intel/compiler/brw_eu_emit.c
> +++ b/src/intel/compiler/brw_eu_emit.c
> @@ -3312,6 +3312,13 @@ brw_find_live_channel(struct brw_codegen *p, struct
> brw_reg dst,
>
> brw_push_insn_state(p);
>
> + /* The flag register is only used on Gen7 in align1 mode, so avoid setting
> +* unnecessary bits in the instruction words, get the information we need
> +* and reset the default flag register.
Maybe mention here that this also allows more instructions to be
compacted. Looks good otherwise:
Reviewed-by: Francisco Jerez
> +*/
> + const unsigned flag_subreg = p->current->flag_subreg;
> + brw_set_default_flag_reg(p, 0, 0);
> +
> if (brw_get_default_access_mode(p) == BRW_ALIGN_1) {
>brw_set_default_mask_control(p, BRW_MASK_DISABLE);
>
> @@ -3345,8 +3352,7 @@ brw_find_live_channel(struct brw_codegen *p, struct
> brw_reg dst,
>*/
> inst = brw_FBL(p, vec1(dst), exec_mask);
>} else {
> - const struct brw_reg flag = brw_flag_reg(p->current->flag_subreg /
> 2,
> - p->current->flag_subreg %
> 2);
> + const struct brw_reg flag = brw_flag_subreg(flag_subreg);
>
> brw_set_default_exec_size(p, BRW_EXECUTE_1);
> brw_MOV(p, retype(flag, BRW_REGISTER_TYPE_UD), brw_imm_ud(0));
> @@ -3366,6 +3372,8 @@ brw_find_live_channel(struct brw_codegen *p, struct
> brw_reg dst,
> brw_inst_set_group(devinfo, inst, lower_size * i + 8 *
> qtr_control);
> brw_inst_set_cond_modifier(devinfo, inst, BRW_CONDITIONAL_Z);
> brw_inst_set_exec_size(devinfo, inst, cvt(lower_size) - 1);
> +brw_inst_set_flag_reg_nr(devinfo, inst, flag_subreg / 2);
> +brw_inst_set_flag_subreg_nr(devinfo, inst, flag_subreg % 2);
> }
>
> /* Find the first bit set in the exec_size-wide portion of the flag
> --
> 2.19.2
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] intel/compiler: Reset default flag register in brw_find_live_channel()
Matt Turner writes:
> emit_uniformize() emits SHADER_OPCODE_FIND_LIVE_CHANNEL with its
> flag_subreg set, so that the IR knows which flag is accessed. However
> the flag is only used on Gen7 in Align1 mode.
>
> To avoid setting unnecessary bits in the instruction words, get the
> information we need and reset the default flag register. This allows
> round-tripping through the assembler/disassembler.
> ---
> src/intel/compiler/brw_eu_emit.c | 16 ++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/src/intel/compiler/brw_eu_emit.c
> b/src/intel/compiler/brw_eu_emit.c
> index 45e2552783b..d05ea506353 100644
> --- a/src/intel/compiler/brw_eu_emit.c
> +++ b/src/intel/compiler/brw_eu_emit.c
> @@ -3312,6 +3312,14 @@ brw_find_live_channel(struct brw_codegen *p, struct
> brw_reg dst,
>
> brw_push_insn_state(p);
>
> + /* The flag register is only used on Gen7 in align1 mode, so avoid setting
> +* unnecessary bits in the instruction words, get the information we need
> +* and reset the default flag register.
> +*/
> + int flag_reg = p->current->flag_subreg / 2;
> + int flag_subreg = p->current->flag_subreg % 2;
You can replace the two lines above with:
+ const unsigned flag_subreg = p->current->flag_subreg;
> + brw_set_default_flag_reg(p, 0, 0);
> +
> if (brw_get_default_access_mode(p) == BRW_ALIGN_1) {
>brw_set_default_mask_control(p, BRW_MASK_DISABLE);
>
> @@ -3345,12 +3353,14 @@ brw_find_live_channel(struct brw_codegen *p, struct
> brw_reg dst,
>*/
> inst = brw_FBL(p, vec1(dst), exec_mask);
>} else {
> - const struct brw_reg flag = brw_flag_reg(p->current->flag_subreg /
> 2,
> - p->current->flag_subreg %
> 2);
> + const struct brw_reg flag = brw_flag_reg(flag_reg, flag_subreg);
so this will just be "brw_flag_subreg(flag_subreg)".
>
> brw_set_default_exec_size(p, BRW_EXECUTE_1);
> brw_MOV(p, retype(flag, BRW_REGISTER_TYPE_UD), brw_imm_ud(0));
>
> + brw_push_insn_state(p);
> + brw_set_default_flag_reg(p, flag_reg, flag_subreg);
> +
No need to push and pop another entry into the default instruction state
stack, you can just "brw_inst_set_flag_*reg_nr()" on the MOV instruction.
> /* Run enough instructions returning zero with execution masking and
>* a conditional modifier enabled in order to get the full execution
>* mask in f1.0. We could use a single 32-wide move here if it
> @@ -3368,6 +3378,8 @@ brw_find_live_channel(struct brw_codegen *p, struct
> brw_reg dst,
> brw_inst_set_exec_size(devinfo, inst, cvt(lower_size) - 1);
> }
>
> + brw_pop_insn_state(p);
> +
> /* Find the first bit set in the exec_size-wide portion of the flag
>* register that was updated by the last sequence of MOV
>* instructions.
> --
> 2.19.2
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] intel/compiler: Reset default flag register in brw_find_live_channel()
On Tue, Jan 22, 2019 at 11:53 AM Francisco Jerez wrote: > > Matt Turner writes: > > > emit_uniformize() emits SHADER_OPCODE_FIND_LIVE_CHANNEL with its > > flag_subreg set, so that the IR knows which flag is accessed. However > > the flag is only used on Gen7 in Align1 mode, and it is used as an > > explicit source and destination. > > > > To avoid setting unnecessary bits in the instruction words, get the > > information we need and reset the default flag register. This allows > > round-tripping through the assembler/disassembler. > > --- > > src/intel/compiler/brw_eu_emit.c | 11 --- > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/src/intel/compiler/brw_eu_emit.c > > b/src/intel/compiler/brw_eu_emit.c > > index 45e2552783b..e6f6d6419d2 100644 > > --- a/src/intel/compiler/brw_eu_emit.c > > +++ b/src/intel/compiler/brw_eu_emit.c > > @@ -3312,6 +3312,14 @@ brw_find_live_channel(struct brw_codegen *p, struct > > brw_reg dst, > > > > brw_push_insn_state(p); > > > > + /* The flag register is only used on Gen7 in align1 mode, so avoid > > setting > > +* unnecessary bits in the instruction words, get the information we > > need > > +* and reset the default flag register. > > +*/ > > + const struct brw_reg flag = brw_flag_reg(p->current->flag_subreg / 2, > > +p->current->flag_subreg % 2); > > + brw_set_default_flag_reg(p, 0, 0); > > + > > I think this is going to break Gen7, because the MOV instructions > emitted in the loop below have conditional mod enabled and won't be > pointing at the right flag register anymore after this change. Crap, I missed that. I'll send an updated patch. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] intel/compiler: Reset default flag register in brw_find_live_channel()
Matt Turner writes:
> emit_uniformize() emits SHADER_OPCODE_FIND_LIVE_CHANNEL with its
> flag_subreg set, so that the IR knows which flag is accessed. However
> the flag is only used on Gen7 in Align1 mode, and it is used as an
> explicit source and destination.
>
> To avoid setting unnecessary bits in the instruction words, get the
> information we need and reset the default flag register. This allows
> round-tripping through the assembler/disassembler.
> ---
> src/intel/compiler/brw_eu_emit.c | 11 ---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/src/intel/compiler/brw_eu_emit.c
> b/src/intel/compiler/brw_eu_emit.c
> index 45e2552783b..e6f6d6419d2 100644
> --- a/src/intel/compiler/brw_eu_emit.c
> +++ b/src/intel/compiler/brw_eu_emit.c
> @@ -3312,6 +3312,14 @@ brw_find_live_channel(struct brw_codegen *p, struct
> brw_reg dst,
>
> brw_push_insn_state(p);
>
> + /* The flag register is only used on Gen7 in align1 mode, so avoid setting
> +* unnecessary bits in the instruction words, get the information we need
> +* and reset the default flag register.
> +*/
> + const struct brw_reg flag = brw_flag_reg(p->current->flag_subreg / 2,
> +p->current->flag_subreg % 2);
> + brw_set_default_flag_reg(p, 0, 0);
> +
I think this is going to break Gen7, because the MOV instructions
emitted in the loop below have conditional mod enabled and won't be
pointing at the right flag register anymore after this change.
> if (brw_get_default_access_mode(p) == BRW_ALIGN_1) {
>brw_set_default_mask_control(p, BRW_MASK_DISABLE);
>
> @@ -3345,9 +3353,6 @@ brw_find_live_channel(struct brw_codegen *p, struct
> brw_reg dst,
>*/
> inst = brw_FBL(p, vec1(dst), exec_mask);
>} else {
> - const struct brw_reg flag = brw_flag_reg(p->current->flag_subreg /
> 2,
> - p->current->flag_subreg %
> 2);
> -
> brw_set_default_exec_size(p, BRW_EXECUTE_1);
> brw_MOV(p, retype(flag, BRW_REGISTER_TYPE_UD), brw_imm_ud(0));
>
> --
> 2.19.2
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
