Re: [Mesa-dev] [PATCH v4 34/40] intel/compiler: validate region restrictions for half-float conversions
Iago Toral writes:
> On Tue, 2019-03-12 at 15:44 -0700, Francisco Jerez wrote:
>> Iago Toral writes:
>>
>> > On Tue, 2019-03-05 at 07:35 +0100, Iago Toral wrote:
>> > > On Mon, 2019-03-04 at 15:36 -0800, Francisco Jerez wrote:
>> > > > Iago Toral writes:
>> > > >
>> > > > > On Fri, 2019-03-01 at 19:04 -0800, Francisco Jerez wrote:
>> > > > > > Iago Toral writes:
>> > > > > >
>> > > > > > > On Thu, 2019-02-28 at 09:54 -0800, Francisco Jerez wrote:
>> > > > > > > > Iago Toral writes:
>> > > > > > > >
>> > > > > > > > > On Wed, 2019-02-27 at 13:47 -0800, Francisco Jerez
>> > > > > > > > > wrote:
>> > > > > > > > > > Iago Toral writes:
>> > > > > > > > > >
>> > > > > > > > > > > On Tue, 2019-02-26 at 14:54 -0800, Francisco
>> > > > > > > > > > > Jerez
>> > > > > > > > > > > wrote:
>> > > > > > > > > > > > Iago Toral Quiroga writes:
>> > > > > > > > > > > >
>> > > > > > > > > > > > > ---
>> > > > > > > > > > > > > src/intel/compiler/brw_eu_validate.c| 6
>> > > > > > > > > > > > > 4
>> > > > > > > > > > > > > -
>> > > > > > > > > > > > > src/intel/compiler/test_eu_validate.cpp |
>> > > > > > > > > > > > > 122
>> > > > > > > > > > > > >
>> > > > > > > > > > > > > 2 files changed, 185 insertions(+), 1
>> > > > > > > > > > > > > deletion(-
>> > > > > > > > > > > > > )
>> > > > > > > > > > > > >
>> > > > > > > > > > > > > diff --git
>> > > > > > > > > > > > > a/src/intel/compiler/brw_eu_validate.c
>> > > > > > > > > > > > > b/src/intel/compiler/brw_eu_validate.c
>> > > > > > > > > > > > > index 000a05cb6ac..203641fecb9 100644
>> > > > > > > > > > > > > --- a/src/intel/compiler/brw_eu_validate.c
>> > > > > > > > > > > > > +++ b/src/intel/compiler/brw_eu_validate.c
>> > > > > > > > > > > > > @@ -531,7 +531,69 @@
>> > > > > > > > > > > > > general_restrictions_based_on_operand_types(c
>> > > > > > > > > > > > > onst
>> > > > > > > > > > > > > struct
>> > > > > > > > > > > > > gen_device_info *devinf
>> > > > > > > > > > > > > exec_type_size == 8 && dst_type_size
>> > > > > > > > > > > > > ==
>> > > > > > > > > > > > > 4)
>> > > > > > > > > > > > >dst_type_size = 8;
>> > > > > > > > > > > > >
>> > > > > > > > > > > > > - if (exec_type_size > dst_type_size) {
>> > > > > > > > > > > > > + /* From the BDW+ PRM:
>> > > > > > > > > > > > > +*
>> > > > > > > > > > > > > +*"There is no direct conversion from
>> > > > > > > > > > > > > HF
>> > > > > > > > > > > > > to
>> > > > > > > > > > > > > DF
>> > > > > > > > > > > > > or
>> > > > > > > > > > > > > DF to
>> > > > > > > > > > > > > HF.
>> > > > > > > > > > > > > +* There is no direct conversion from
>> > > > > > > > > > > > > HF
>> > > > > > > > > > > > > to
>> > > > > > > > > > > > > Q/UQ or
>> > > > > > > > > > > > > Q/UQ to
>> > > > > > > > > > > > > HF."
>> > > > > > > > > > > > > +*/
>> > > > > > > > > > > > > + enum brw_reg_type src0_type =
>> > > > > > > > > > > > > brw_inst_src0_type(devinfo,
>> > > > > > > > > > > > > inst);
>> > > > > > > > > > > > > + ERROR_IF(brw_inst_opcode(devinfo, inst)
>> > > > > > > > > > > > > ==
>> > > > > > > > > > > > > BRW_OPCODE_MOV
>> > > > > > > > > > > > > &&
>> > > > > > > > > > > >
>> > > > > > > > > > > > Why is only the MOV instruction handled here
>> > > > > > > > > > > > and
>> > > > > > > > > > > > below? Aren't
>> > > > > > > > > > > > other
>> > > > > > > > > > > > instructions able to do implicit
>> > > > > > > > > > > > conversions? Probably
>> > > > > > > > > > > > means
>> > > > > > > > > > > > you
>> > > > > > > > > > > > need
>> > > > > > > > > > > > to deal with two sources rather than one.
>> > > > > > > > > > >
>> > > > > > > > > > > This comes from the programming notes of the MOV
>> > > > > > > > > > > instruction
>> > > > > > > > > > > (Volume
>> > > > > > > > > > > 2a, Command Reference - Instructions - MOV), so
>> > > > > > > > > > > it is
>> > > > > > > > > > > described
>> > > > > > > > > > > specifically for the MOV instruction. I should
>> > > > > > > > > > > probably
>> > > > > > > > > > > have
>> > > > > > > > > > > made
>> > > > > > > > > > > this
>> > > > > > > > > > > clear in the comment.
>> > > > > > > > > > >
>> > > > > > > > > >
>> > > > > > > > > > Maybe the one above is specified in the MOV page
>> > > > > > > > > > only,
>> > > > > > > > > > probably
>> > > > > > > > > > due
>> > > > > > > > > > to
>> > > > > > > > > > an oversight (If these restrictions were really
>> > > > > > > > > > specific
>> > > > > > > > > > to
>> > > > > > > > > > the
>> > > > > > > > > > MOV
>> > > > > > > > > > instruction, what would prevent you from
>> > > > > > > > > > implementing
>> > > > > > > > > > such
>> > > > > > > > > > conversions
>> > > > > > > > > > through a different instruction? E.g. "ADD dst:df,
>> > > > > > > > > > src:hf,
>> > > > > > > > > > 0"
>> > > > > > > > > > which
>> > > > > > > > > > would be substantially more efficient than what
>> > > > > > > > > > you're
>> > > > > > >
Re: [Mesa-dev] [PATCH v4 34/40] intel/compiler: validate region restrictions for half-float conversions
On Tue, 2019-03-12 at 15:46 -0700, Francisco Jerez wrote:
> Iago Toral writes:
>
> > On Wed, 2019-03-06 at 09:21 +0100, Iago Toral wrote:
> > > On Tue, 2019-03-05 at 07:35 +0100, Iago Toral wrote:
> > > > On Mon, 2019-03-04 at 15:36 -0800, Francisco Jerez wrote:
> > > > > Iago Toral writes:
> > > > >
> > > > > > On Fri, 2019-03-01 at 19:04 -0800, Francisco Jerez wrote:
> > > > > > > Iago Toral writes:
> > > > > > >
> > > > > > > > On Thu, 2019-02-28 at 09:54 -0800, Francisco Jerez
> > > > > > > > wrote:
> > > > > > > > > Iago Toral writes:
> > > > > > > > >
> > > > > > > > > > On Wed, 2019-02-27 at 13:47 -0800, Francisco Jerez
> > > > > > > > > > wrote:
> > > > > > > > > > > Iago Toral writes:
> > > > > > > > > > >
> > > > > > > > > > > > On Tue, 2019-02-26 at 14:54 -0800, Francisco
> > > > > > > > > > > > Jerez
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > > Iago Toral Quiroga
> > > > > > > > > > > > > writes:
> > > > > > > > > > > > >
> > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > src/intel/compiler/brw_eu_validate.c|
> > > > > > > > > > > > > > 64
> > > > > > > > > > > > > > -
> > > > > > > > > > > > > > src/intel/compiler/test_eu_validate.cpp |
> > > > > > > > > > > > > > 122
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > 2 files changed, 185 insertions(+), 1
> > > > > > > > > > > > > > deletion(-
> > > > > > > > > > > > > > )
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > diff --git
> > > > > > > > > > > > > > a/src/intel/compiler/brw_eu_validate.c
> > > > > > > > > > > > > > b/src/intel/compiler/brw_eu_validate.c
> > > > > > > > > > > > > > index 000a05cb6ac..203641fecb9 100644
> > > > > > > > > > > > > > --- a/src/intel/compiler/brw_eu_validate.c
> > > > > > > > > > > > > > +++ b/src/intel/compiler/brw_eu_validate.c
> > > > > > > > > > > > > > @@ -531,7 +531,69 @@
> > > > > > > > > > > > > > general_restrictions_based_on_operand_types
> > > > > > > > > > > > > > (con
> > > > > > > > > > > > > > st
> > > > > > > > > > > > > > struct
> > > > > > > > > > > > > > gen_device_info *devinf
> > > > > > > > > > > > > > exec_type_size == 8 &&
> > > > > > > > > > > > > > dst_type_size ==
> > > > > > > > > > > > > > 4)
> > > > > > > > > > > > > >dst_type_size = 8;
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > - if (exec_type_size > dst_type_size) {
> > > > > > > > > > > > > > + /* From the BDW+ PRM:
> > > > > > > > > > > > > > +*
> > > > > > > > > > > > > > +*"There is no direct conversion
> > > > > > > > > > > > > > from
> > > > > > > > > > > > > > HF
> > > > > > > > > > > > > > to
> > > > > > > > > > > > > > DF
> > > > > > > > > > > > > > or
> > > > > > > > > > > > > > DF to
> > > > > > > > > > > > > > HF.
> > > > > > > > > > > > > > +* There is no direct conversion
> > > > > > > > > > > > > > from
> > > > > > > > > > > > > > HF
> > > > > > > > > > > > > > to
> > > > > > > > > > > > > > Q/UQ or
> > > > > > > > > > > > > > Q/UQ to
> > > > > > > > > > > > > > HF."
> > > > > > > > > > > > > > +*/
> > > > > > > > > > > > > > + enum brw_reg_type src0_type =
> > > > > > > > > > > > > > brw_inst_src0_type(devinfo,
> > > > > > > > > > > > > > inst);
> > > > > > > > > > > > > > + ERROR_IF(brw_inst_opcode(devinfo, inst)
> > > > > > > > > > > > > > ==
> > > > > > > > > > > > > > BRW_OPCODE_MOV
> > > > > > > > > > > > > > &&
> > > > > > > > > > > > >
> > > > > > > > > > > > > Why is only the MOV instruction handled here
> > > > > > > > > > > > > and
> > > > > > > > > > > > > below? Aren't
> > > > > > > > > > > > > other
> > > > > > > > > > > > > instructions able to do implicit
> > > > > > > > > > > > > conversions? Probably
> > > > > > > > > > > > > means
> > > > > > > > > > > > > you
> > > > > > > > > > > > > need
> > > > > > > > > > > > > to deal with two sources rather than one.
> > > > > > > > > > > >
> > > > > > > > > > > > This comes from the programming notes of the
> > > > > > > > > > > > MOV
> > > > > > > > > > > > instruction
> > > > > > > > > > > > (Volume
> > > > > > > > > > > > 2a, Command Reference - Instructions - MOV), so
> > > > > > > > > > > > it
> > > > > > > > > > > > is
> > > > > > > > > > > > described
> > > > > > > > > > > > specifically for the MOV instruction. I should
> > > > > > > > > > > > probably
> > > > > > > > > > > > have
> > > > > > > > > > > > made
> > > > > > > > > > > > this
> > > > > > > > > > > > clear in the comment.
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Maybe the one above is specified in the MOV page
> > > > > > > > > > > only,
> > > > > > > > > > > probably
> > > > > > > > > > > due
> > > > > > > > > > > to
> > > > > > > > > > > an oversight (If these restrictions were really
> > > > > > > > > > > specific
> > > > > > > > > > > to
> > > > > > > > > > > the
> > > > > > > > > > > MOV
> > > > > > > > > > > instruction, what would prevent you from
> > > > > > > > > > > implementing
> > > >
Re: [Mesa-dev] [PATCH v4 34/40] intel/compiler: validate region restrictions for half-float conversions
On Tue, 2019-03-12 at 15:44 -0700, Francisco Jerez wrote:
> Iago Toral writes:
>
> > On Tue, 2019-03-05 at 07:35 +0100, Iago Toral wrote:
> > > On Mon, 2019-03-04 at 15:36 -0800, Francisco Jerez wrote:
> > > > Iago Toral writes:
> > > >
> > > > > On Fri, 2019-03-01 at 19:04 -0800, Francisco Jerez wrote:
> > > > > > Iago Toral writes:
> > > > > >
> > > > > > > On Thu, 2019-02-28 at 09:54 -0800, Francisco Jerez wrote:
> > > > > > > > Iago Toral writes:
> > > > > > > >
> > > > > > > > > On Wed, 2019-02-27 at 13:47 -0800, Francisco Jerez
> > > > > > > > > wrote:
> > > > > > > > > > Iago Toral writes:
> > > > > > > > > >
> > > > > > > > > > > On Tue, 2019-02-26 at 14:54 -0800, Francisco
> > > > > > > > > > > Jerez
> > > > > > > > > > > wrote:
> > > > > > > > > > > > Iago Toral Quiroga writes:
> > > > > > > > > > > >
> > > > > > > > > > > > > ---
> > > > > > > > > > > > > src/intel/compiler/brw_eu_validate.c| 6
> > > > > > > > > > > > > 4
> > > > > > > > > > > > > -
> > > > > > > > > > > > > src/intel/compiler/test_eu_validate.cpp |
> > > > > > > > > > > > > 122
> > > > > > > > > > > > >
> > > > > > > > > > > > > 2 files changed, 185 insertions(+), 1
> > > > > > > > > > > > > deletion(-
> > > > > > > > > > > > > )
> > > > > > > > > > > > >
> > > > > > > > > > > > > diff --git
> > > > > > > > > > > > > a/src/intel/compiler/brw_eu_validate.c
> > > > > > > > > > > > > b/src/intel/compiler/brw_eu_validate.c
> > > > > > > > > > > > > index 000a05cb6ac..203641fecb9 100644
> > > > > > > > > > > > > --- a/src/intel/compiler/brw_eu_validate.c
> > > > > > > > > > > > > +++ b/src/intel/compiler/brw_eu_validate.c
> > > > > > > > > > > > > @@ -531,7 +531,69 @@
> > > > > > > > > > > > > general_restrictions_based_on_operand_types(c
> > > > > > > > > > > > > onst
> > > > > > > > > > > > > struct
> > > > > > > > > > > > > gen_device_info *devinf
> > > > > > > > > > > > > exec_type_size == 8 && dst_type_size
> > > > > > > > > > > > > ==
> > > > > > > > > > > > > 4)
> > > > > > > > > > > > >dst_type_size = 8;
> > > > > > > > > > > > >
> > > > > > > > > > > > > - if (exec_type_size > dst_type_size) {
> > > > > > > > > > > > > + /* From the BDW+ PRM:
> > > > > > > > > > > > > +*
> > > > > > > > > > > > > +*"There is no direct conversion from
> > > > > > > > > > > > > HF
> > > > > > > > > > > > > to
> > > > > > > > > > > > > DF
> > > > > > > > > > > > > or
> > > > > > > > > > > > > DF to
> > > > > > > > > > > > > HF.
> > > > > > > > > > > > > +* There is no direct conversion from
> > > > > > > > > > > > > HF
> > > > > > > > > > > > > to
> > > > > > > > > > > > > Q/UQ or
> > > > > > > > > > > > > Q/UQ to
> > > > > > > > > > > > > HF."
> > > > > > > > > > > > > +*/
> > > > > > > > > > > > > + enum brw_reg_type src0_type =
> > > > > > > > > > > > > brw_inst_src0_type(devinfo,
> > > > > > > > > > > > > inst);
> > > > > > > > > > > > > + ERROR_IF(brw_inst_opcode(devinfo, inst)
> > > > > > > > > > > > > ==
> > > > > > > > > > > > > BRW_OPCODE_MOV
> > > > > > > > > > > > > &&
> > > > > > > > > > > >
> > > > > > > > > > > > Why is only the MOV instruction handled here
> > > > > > > > > > > > and
> > > > > > > > > > > > below? Aren't
> > > > > > > > > > > > other
> > > > > > > > > > > > instructions able to do implicit
> > > > > > > > > > > > conversions? Probably
> > > > > > > > > > > > means
> > > > > > > > > > > > you
> > > > > > > > > > > > need
> > > > > > > > > > > > to deal with two sources rather than one.
> > > > > > > > > > >
> > > > > > > > > > > This comes from the programming notes of the MOV
> > > > > > > > > > > instruction
> > > > > > > > > > > (Volume
> > > > > > > > > > > 2a, Command Reference - Instructions - MOV), so
> > > > > > > > > > > it is
> > > > > > > > > > > described
> > > > > > > > > > > specifically for the MOV instruction. I should
> > > > > > > > > > > probably
> > > > > > > > > > > have
> > > > > > > > > > > made
> > > > > > > > > > > this
> > > > > > > > > > > clear in the comment.
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Maybe the one above is specified in the MOV page
> > > > > > > > > > only,
> > > > > > > > > > probably
> > > > > > > > > > due
> > > > > > > > > > to
> > > > > > > > > > an oversight (If these restrictions were really
> > > > > > > > > > specific
> > > > > > > > > > to
> > > > > > > > > > the
> > > > > > > > > > MOV
> > > > > > > > > > instruction, what would prevent you from
> > > > > > > > > > implementing
> > > > > > > > > > such
> > > > > > > > > > conversions
> > > > > > > > > > through a different instruction? E.g. "ADD dst:df,
> > > > > > > > > > src:hf,
> > > > > > > > > > 0"
> > > > > > > > > > which
> > > > > > > > > > would be substantially more efficient than what
> > > > > > > > > > you're
> > > > > > > > > > doing
> > > > > > > > > > in
> > > > > > > > > > PATCH
> > > > > > > > > > 02)
> > > > > > > > >
> > > > > > > > > Instructions that t
Re: [Mesa-dev] [PATCH v4 34/40] intel/compiler: validate region restrictions for half-float conversions
Francisco Jerez writes:
> Iago Toral writes:
>
>> On Tue, 2019-03-05 at 07:35 +0100, Iago Toral wrote:
>>> On Mon, 2019-03-04 at 15:36 -0800, Francisco Jerez wrote:
>>> > Iago Toral writes:
>>> >
>>> > > On Fri, 2019-03-01 at 19:04 -0800, Francisco Jerez wrote:
>>> > > > Iago Toral writes:
>>> > > >
>>> > > > > On Thu, 2019-02-28 at 09:54 -0800, Francisco Jerez wrote:
>>> > > > > > Iago Toral writes:
>>> > > > > >
>>> > > > > > > On Wed, 2019-02-27 at 13:47 -0800, Francisco Jerez wrote:
>>> > > > > > > > Iago Toral writes:
>>> > > > > > > >
>>> > > > > > > > > On Tue, 2019-02-26 at 14:54 -0800, Francisco Jerez
>>> > > > > > > > > wrote:
>>> > > > > > > > > > Iago Toral Quiroga writes:
>>> > > > > > > > > >
>>> > > > > > > > > > > ---
>>> > > > > > > > > > > src/intel/compiler/brw_eu_validate.c| 64
>>> > > > > > > > > > > -
>>> > > > > > > > > > > src/intel/compiler/test_eu_validate.cpp | 122
>>> > > > > > > > > > >
>>> > > > > > > > > > > 2 files changed, 185 insertions(+), 1 deletion(-
>>> > > > > > > > > > > )
>>> > > > > > > > > > >
>>> > > > > > > > > > > diff --git a/src/intel/compiler/brw_eu_validate.c
>>> > > > > > > > > > > b/src/intel/compiler/brw_eu_validate.c
>>> > > > > > > > > > > index 000a05cb6ac..203641fecb9 100644
>>> > > > > > > > > > > --- a/src/intel/compiler/brw_eu_validate.c
>>> > > > > > > > > > > +++ b/src/intel/compiler/brw_eu_validate.c
>>> > > > > > > > > > > @@ -531,7 +531,69 @@
>>> > > > > > > > > > > general_restrictions_based_on_operand_types(const
>>> > > > > > > > > > > struct
>>> > > > > > > > > > > gen_device_info *devinf
>>> > > > > > > > > > > exec_type_size == 8 && dst_type_size ==
>>> > > > > > > > > > > 4)
>>> > > > > > > > > > >dst_type_size = 8;
>>> > > > > > > > > > >
>>> > > > > > > > > > > - if (exec_type_size > dst_type_size) {
>>> > > > > > > > > > > + /* From the BDW+ PRM:
>>> > > > > > > > > > > +*
>>> > > > > > > > > > > +*"There is no direct conversion from HF
>>> > > > > > > > > > > to
>>> > > > > > > > > > > DF
>>> > > > > > > > > > > or
>>> > > > > > > > > > > DF to
>>> > > > > > > > > > > HF.
>>> > > > > > > > > > > +* There is no direct conversion from HF
>>> > > > > > > > > > > to
>>> > > > > > > > > > > Q/UQ or
>>> > > > > > > > > > > Q/UQ to
>>> > > > > > > > > > > HF."
>>> > > > > > > > > > > +*/
>>> > > > > > > > > > > + enum brw_reg_type src0_type =
>>> > > > > > > > > > > brw_inst_src0_type(devinfo,
>>> > > > > > > > > > > inst);
>>> > > > > > > > > > > + ERROR_IF(brw_inst_opcode(devinfo, inst) ==
>>> > > > > > > > > > > BRW_OPCODE_MOV
>>> > > > > > > > > > > &&
>>> > > > > > > > > >
>>> > > > > > > > > > Why is only the MOV instruction handled here and
>>> > > > > > > > > > below? Aren't
>>> > > > > > > > > > other
>>> > > > > > > > > > instructions able to do implicit
>>> > > > > > > > > > conversions? Probably
>>> > > > > > > > > > means
>>> > > > > > > > > > you
>>> > > > > > > > > > need
>>> > > > > > > > > > to deal with two sources rather than one.
>>> > > > > > > > >
>>> > > > > > > > > This comes from the programming notes of the MOV
>>> > > > > > > > > instruction
>>> > > > > > > > > (Volume
>>> > > > > > > > > 2a, Command Reference - Instructions - MOV), so it is
>>> > > > > > > > > described
>>> > > > > > > > > specifically for the MOV instruction. I should
>>> > > > > > > > > probably
>>> > > > > > > > > have
>>> > > > > > > > > made
>>> > > > > > > > > this
>>> > > > > > > > > clear in the comment.
>>> > > > > > > > >
>>> > > > > > > >
>>> > > > > > > > Maybe the one above is specified in the MOV page only,
>>> > > > > > > > probably
>>> > > > > > > > due
>>> > > > > > > > to
>>> > > > > > > > an oversight (If these restrictions were really
>>> > > > > > > > specific
>>> > > > > > > > to
>>> > > > > > > > the
>>> > > > > > > > MOV
>>> > > > > > > > instruction, what would prevent you from implementing
>>> > > > > > > > such
>>> > > > > > > > conversions
>>> > > > > > > > through a different instruction? E.g. "ADD dst:df,
>>> > > > > > > > src:hf,
>>> > > > > > > > 0"
>>> > > > > > > > which
>>> > > > > > > > would be substantially more efficient than what you're
>>> > > > > > > > doing
>>> > > > > > > > in
>>> > > > > > > > PATCH
>>> > > > > > > > 02)
>>> > > > > > >
>>> > > > > > > Instructions that take HF can only be strictly HF or mix
>>> > > > > > > F
>>> > > > > > > and
>>> > > > > > > HF
>>> > > > > > > (mixed mode float), with MOV being the only exception.
>>> > > > > > > That
>>> > > > > > > means
>>> > > > > > > that
>>> > > > > > > any instruction like the one you use above are invalid.
>>> > > > > > > Maybe
>>> > > > > > > we
>>> > > > > > > should
>>> > > > > > > validate explicitly that instructions that use HF are
>>> > > > > > > strictly
>>> > > > > > > HF
>>> > > > > > > or
>>> > > > > > > mixed-float mode only?
>>> > > > > > >
>>> > > > > >
>>> > > > > > So you're acknowledging that the conversio
Re: [Mesa-dev] [PATCH v4 34/40] intel/compiler: validate region restrictions for half-float conversions
Iago Toral writes:
> On Wed, 2019-03-06 at 09:21 +0100, Iago Toral wrote:
>> On Tue, 2019-03-05 at 07:35 +0100, Iago Toral wrote:
>> > On Mon, 2019-03-04 at 15:36 -0800, Francisco Jerez wrote:
>> > > Iago Toral writes:
>> > >
>> > > > On Fri, 2019-03-01 at 19:04 -0800, Francisco Jerez wrote:
>> > > > > Iago Toral writes:
>> > > > >
>> > > > > > On Thu, 2019-02-28 at 09:54 -0800, Francisco Jerez wrote:
>> > > > > > > Iago Toral writes:
>> > > > > > >
>> > > > > > > > On Wed, 2019-02-27 at 13:47 -0800, Francisco Jerez
>> > > > > > > > wrote:
>> > > > > > > > > Iago Toral writes:
>> > > > > > > > >
>> > > > > > > > > > On Tue, 2019-02-26 at 14:54 -0800, Francisco Jerez
>> > > > > > > > > > wrote:
>> > > > > > > > > > > Iago Toral Quiroga writes:
>> > > > > > > > > > >
>> > > > > > > > > > > > ---
>> > > > > > > > > > > > src/intel/compiler/brw_eu_validate.c| 64
>> > > > > > > > > > > > -
>> > > > > > > > > > > > src/intel/compiler/test_eu_validate.cpp | 122
>> > > > > > > > > > > >
>> > > > > > > > > > > > 2 files changed, 185 insertions(+), 1
>> > > > > > > > > > > > deletion(-
>> > > > > > > > > > > > )
>> > > > > > > > > > > >
>> > > > > > > > > > > > diff --git
>> > > > > > > > > > > > a/src/intel/compiler/brw_eu_validate.c
>> > > > > > > > > > > > b/src/intel/compiler/brw_eu_validate.c
>> > > > > > > > > > > > index 000a05cb6ac..203641fecb9 100644
>> > > > > > > > > > > > --- a/src/intel/compiler/brw_eu_validate.c
>> > > > > > > > > > > > +++ b/src/intel/compiler/brw_eu_validate.c
>> > > > > > > > > > > > @@ -531,7 +531,69 @@
>> > > > > > > > > > > > general_restrictions_based_on_operand_types(con
>> > > > > > > > > > > > st
>> > > > > > > > > > > > struct
>> > > > > > > > > > > > gen_device_info *devinf
>> > > > > > > > > > > > exec_type_size == 8 && dst_type_size ==
>> > > > > > > > > > > > 4)
>> > > > > > > > > > > >dst_type_size = 8;
>> > > > > > > > > > > >
>> > > > > > > > > > > > - if (exec_type_size > dst_type_size) {
>> > > > > > > > > > > > + /* From the BDW+ PRM:
>> > > > > > > > > > > > +*
>> > > > > > > > > > > > +*"There is no direct conversion from
>> > > > > > > > > > > > HF
>> > > > > > > > > > > > to
>> > > > > > > > > > > > DF
>> > > > > > > > > > > > or
>> > > > > > > > > > > > DF to
>> > > > > > > > > > > > HF.
>> > > > > > > > > > > > +* There is no direct conversion from
>> > > > > > > > > > > > HF
>> > > > > > > > > > > > to
>> > > > > > > > > > > > Q/UQ or
>> > > > > > > > > > > > Q/UQ to
>> > > > > > > > > > > > HF."
>> > > > > > > > > > > > +*/
>> > > > > > > > > > > > + enum brw_reg_type src0_type =
>> > > > > > > > > > > > brw_inst_src0_type(devinfo,
>> > > > > > > > > > > > inst);
>> > > > > > > > > > > > + ERROR_IF(brw_inst_opcode(devinfo, inst) ==
>> > > > > > > > > > > > BRW_OPCODE_MOV
>> > > > > > > > > > > > &&
>> > > > > > > > > > >
>> > > > > > > > > > > Why is only the MOV instruction handled here and
>> > > > > > > > > > > below? Aren't
>> > > > > > > > > > > other
>> > > > > > > > > > > instructions able to do implicit
>> > > > > > > > > > > conversions? Probably
>> > > > > > > > > > > means
>> > > > > > > > > > > you
>> > > > > > > > > > > need
>> > > > > > > > > > > to deal with two sources rather than one.
>> > > > > > > > > >
>> > > > > > > > > > This comes from the programming notes of the MOV
>> > > > > > > > > > instruction
>> > > > > > > > > > (Volume
>> > > > > > > > > > 2a, Command Reference - Instructions - MOV), so it
>> > > > > > > > > > is
>> > > > > > > > > > described
>> > > > > > > > > > specifically for the MOV instruction. I should
>> > > > > > > > > > probably
>> > > > > > > > > > have
>> > > > > > > > > > made
>> > > > > > > > > > this
>> > > > > > > > > > clear in the comment.
>> > > > > > > > > >
>> > > > > > > > >
>> > > > > > > > > Maybe the one above is specified in the MOV page
>> > > > > > > > > only,
>> > > > > > > > > probably
>> > > > > > > > > due
>> > > > > > > > > to
>> > > > > > > > > an oversight (If these restrictions were really
>> > > > > > > > > specific
>> > > > > > > > > to
>> > > > > > > > > the
>> > > > > > > > > MOV
>> > > > > > > > > instruction, what would prevent you from implementing
>> > > > > > > > > such
>> > > > > > > > > conversions
>> > > > > > > > > through a different instruction? E.g. "ADD dst:df,
>> > > > > > > > > src:hf,
>> > > > > > > > > 0"
>> > > > > > > > > which
>> > > > > > > > > would be substantially more efficient than what
>> > > > > > > > > you're
>> > > > > > > > > doing
>> > > > > > > > > in
>> > > > > > > > > PATCH
>> > > > > > > > > 02)
>> > > > > > > >
>> > > > > > > > Instructions that take HF can only be strictly HF or
>> > > > > > > > mix
>> > > > > > > > F
>> > > > > > > > and
>> > > > > > > > HF
>> > > > > > > > (mixed mode float), with MOV being the only exception.
>> > > > > > > > That
>> > > > > > > > means
>> > > > > > > > that
>> > > > > > > > any instruction like t
Re: [Mesa-dev] [PATCH v4 34/40] intel/compiler: validate region restrictions for half-float conversions
Iago Toral writes:
> On Tue, 2019-03-05 at 07:35 +0100, Iago Toral wrote:
>> On Mon, 2019-03-04 at 15:36 -0800, Francisco Jerez wrote:
>> > Iago Toral writes:
>> >
>> > > On Fri, 2019-03-01 at 19:04 -0800, Francisco Jerez wrote:
>> > > > Iago Toral writes:
>> > > >
>> > > > > On Thu, 2019-02-28 at 09:54 -0800, Francisco Jerez wrote:
>> > > > > > Iago Toral writes:
>> > > > > >
>> > > > > > > On Wed, 2019-02-27 at 13:47 -0800, Francisco Jerez wrote:
>> > > > > > > > Iago Toral writes:
>> > > > > > > >
>> > > > > > > > > On Tue, 2019-02-26 at 14:54 -0800, Francisco Jerez
>> > > > > > > > > wrote:
>> > > > > > > > > > Iago Toral Quiroga writes:
>> > > > > > > > > >
>> > > > > > > > > > > ---
>> > > > > > > > > > > src/intel/compiler/brw_eu_validate.c| 64
>> > > > > > > > > > > -
>> > > > > > > > > > > src/intel/compiler/test_eu_validate.cpp | 122
>> > > > > > > > > > >
>> > > > > > > > > > > 2 files changed, 185 insertions(+), 1 deletion(-
>> > > > > > > > > > > )
>> > > > > > > > > > >
>> > > > > > > > > > > diff --git a/src/intel/compiler/brw_eu_validate.c
>> > > > > > > > > > > b/src/intel/compiler/brw_eu_validate.c
>> > > > > > > > > > > index 000a05cb6ac..203641fecb9 100644
>> > > > > > > > > > > --- a/src/intel/compiler/brw_eu_validate.c
>> > > > > > > > > > > +++ b/src/intel/compiler/brw_eu_validate.c
>> > > > > > > > > > > @@ -531,7 +531,69 @@
>> > > > > > > > > > > general_restrictions_based_on_operand_types(const
>> > > > > > > > > > > struct
>> > > > > > > > > > > gen_device_info *devinf
>> > > > > > > > > > > exec_type_size == 8 && dst_type_size ==
>> > > > > > > > > > > 4)
>> > > > > > > > > > >dst_type_size = 8;
>> > > > > > > > > > >
>> > > > > > > > > > > - if (exec_type_size > dst_type_size) {
>> > > > > > > > > > > + /* From the BDW+ PRM:
>> > > > > > > > > > > +*
>> > > > > > > > > > > +*"There is no direct conversion from HF
>> > > > > > > > > > > to
>> > > > > > > > > > > DF
>> > > > > > > > > > > or
>> > > > > > > > > > > DF to
>> > > > > > > > > > > HF.
>> > > > > > > > > > > +* There is no direct conversion from HF
>> > > > > > > > > > > to
>> > > > > > > > > > > Q/UQ or
>> > > > > > > > > > > Q/UQ to
>> > > > > > > > > > > HF."
>> > > > > > > > > > > +*/
>> > > > > > > > > > > + enum brw_reg_type src0_type =
>> > > > > > > > > > > brw_inst_src0_type(devinfo,
>> > > > > > > > > > > inst);
>> > > > > > > > > > > + ERROR_IF(brw_inst_opcode(devinfo, inst) ==
>> > > > > > > > > > > BRW_OPCODE_MOV
>> > > > > > > > > > > &&
>> > > > > > > > > >
>> > > > > > > > > > Why is only the MOV instruction handled here and
>> > > > > > > > > > below? Aren't
>> > > > > > > > > > other
>> > > > > > > > > > instructions able to do implicit
>> > > > > > > > > > conversions? Probably
>> > > > > > > > > > means
>> > > > > > > > > > you
>> > > > > > > > > > need
>> > > > > > > > > > to deal with two sources rather than one.
>> > > > > > > > >
>> > > > > > > > > This comes from the programming notes of the MOV
>> > > > > > > > > instruction
>> > > > > > > > > (Volume
>> > > > > > > > > 2a, Command Reference - Instructions - MOV), so it is
>> > > > > > > > > described
>> > > > > > > > > specifically for the MOV instruction. I should
>> > > > > > > > > probably
>> > > > > > > > > have
>> > > > > > > > > made
>> > > > > > > > > this
>> > > > > > > > > clear in the comment.
>> > > > > > > > >
>> > > > > > > >
>> > > > > > > > Maybe the one above is specified in the MOV page only,
>> > > > > > > > probably
>> > > > > > > > due
>> > > > > > > > to
>> > > > > > > > an oversight (If these restrictions were really
>> > > > > > > > specific
>> > > > > > > > to
>> > > > > > > > the
>> > > > > > > > MOV
>> > > > > > > > instruction, what would prevent you from implementing
>> > > > > > > > such
>> > > > > > > > conversions
>> > > > > > > > through a different instruction? E.g. "ADD dst:df,
>> > > > > > > > src:hf,
>> > > > > > > > 0"
>> > > > > > > > which
>> > > > > > > > would be substantially more efficient than what you're
>> > > > > > > > doing
>> > > > > > > > in
>> > > > > > > > PATCH
>> > > > > > > > 02)
>> > > > > > >
>> > > > > > > Instructions that take HF can only be strictly HF or mix
>> > > > > > > F
>> > > > > > > and
>> > > > > > > HF
>> > > > > > > (mixed mode float), with MOV being the only exception.
>> > > > > > > That
>> > > > > > > means
>> > > > > > > that
>> > > > > > > any instruction like the one you use above are invalid.
>> > > > > > > Maybe
>> > > > > > > we
>> > > > > > > should
>> > > > > > > validate explicitly that instructions that use HF are
>> > > > > > > strictly
>> > > > > > > HF
>> > > > > > > or
>> > > > > > > mixed-float mode only?
>> > > > > > >
>> > > > > >
>> > > > > > So you're acknowledging that the conversions checked above
>> > > > > > are
>> > > > > > illegal
>> > > > > > whether the instruction is a MOV or something else. Where
>> > > > > > are we
>> > >
Re: [Mesa-dev] [PATCH v4 34/40] intel/compiler: validate region restrictions for half-float conversions
Iago Toral writes:
> On Mon, 2019-03-04 at 15:36 -0800, Francisco Jerez wrote:
>> Iago Toral writes:
>>
>> > On Fri, 2019-03-01 at 19:04 -0800, Francisco Jerez wrote:
>> > > Iago Toral writes:
>> > >
>> > > > On Thu, 2019-02-28 at 09:54 -0800, Francisco Jerez wrote:
>> > > > > Iago Toral writes:
>> > > > >
>> > > > > > On Wed, 2019-02-27 at 13:47 -0800, Francisco Jerez wrote:
>> > > > > > > Iago Toral writes:
>> > > > > > >
>> > > > > > > > On Tue, 2019-02-26 at 14:54 -0800, Francisco Jerez
>> > > > > > > > wrote:
>> > > > > > > > > Iago Toral Quiroga writes:
>> > > > > > > > >
>> > > > > > > > > > ---
>> > > > > > > > > > src/intel/compiler/brw_eu_validate.c| 64
>> > > > > > > > > > -
>> > > > > > > > > > src/intel/compiler/test_eu_validate.cpp | 122
>> > > > > > > > > >
>> > > > > > > > > > 2 files changed, 185 insertions(+), 1 deletion(-)
>> > > > > > > > > >
>> > > > > > > > > > diff --git a/src/intel/compiler/brw_eu_validate.c
>> > > > > > > > > > b/src/intel/compiler/brw_eu_validate.c
>> > > > > > > > > > index 000a05cb6ac..203641fecb9 100644
>> > > > > > > > > > --- a/src/intel/compiler/brw_eu_validate.c
>> > > > > > > > > > +++ b/src/intel/compiler/brw_eu_validate.c
>> > > > > > > > > > @@ -531,7 +531,69 @@
>> > > > > > > > > > general_restrictions_based_on_operand_types(const
>> > > > > > > > > > struct
>> > > > > > > > > > gen_device_info *devinf
>> > > > > > > > > > exec_type_size == 8 && dst_type_size == 4)
>> > > > > > > > > >dst_type_size = 8;
>> > > > > > > > > >
>> > > > > > > > > > - if (exec_type_size > dst_type_size) {
>> > > > > > > > > > + /* From the BDW+ PRM:
>> > > > > > > > > > +*
>> > > > > > > > > > +*"There is no direct conversion from HF to
>> > > > > > > > > > DF
>> > > > > > > > > > or
>> > > > > > > > > > DF to
>> > > > > > > > > > HF.
>> > > > > > > > > > +* There is no direct conversion from HF to
>> > > > > > > > > > Q/UQ or
>> > > > > > > > > > Q/UQ to
>> > > > > > > > > > HF."
>> > > > > > > > > > +*/
>> > > > > > > > > > + enum brw_reg_type src0_type =
>> > > > > > > > > > brw_inst_src0_type(devinfo,
>> > > > > > > > > > inst);
>> > > > > > > > > > + ERROR_IF(brw_inst_opcode(devinfo, inst) ==
>> > > > > > > > > > BRW_OPCODE_MOV
>> > > > > > > > > > &&
>> > > > > > > > >
>> > > > > > > > > Why is only the MOV instruction handled here and
>> > > > > > > > > below? Aren't
>> > > > > > > > > other
>> > > > > > > > > instructions able to do implicit
>> > > > > > > > > conversions? Probably
>> > > > > > > > > means
>> > > > > > > > > you
>> > > > > > > > > need
>> > > > > > > > > to deal with two sources rather than one.
>> > > > > > > >
>> > > > > > > > This comes from the programming notes of the MOV
>> > > > > > > > instruction
>> > > > > > > > (Volume
>> > > > > > > > 2a, Command Reference - Instructions - MOV), so it is
>> > > > > > > > described
>> > > > > > > > specifically for the MOV instruction. I should probably
>> > > > > > > > have
>> > > > > > > > made
>> > > > > > > > this
>> > > > > > > > clear in the comment.
>> > > > > > > >
>> > > > > > >
>> > > > > > > Maybe the one above is specified in the MOV page only,
>> > > > > > > probably
>> > > > > > > due
>> > > > > > > to
>> > > > > > > an oversight (If these restrictions were really specific
>> > > > > > > to
>> > > > > > > the
>> > > > > > > MOV
>> > > > > > > instruction, what would prevent you from implementing
>> > > > > > > such
>> > > > > > > conversions
>> > > > > > > through a different instruction? E.g. "ADD dst:df,
>> > > > > > > src:hf,
>> > > > > > > 0"
>> > > > > > > which
>> > > > > > > would be substantially more efficient than what you're
>> > > > > > > doing
>> > > > > > > in
>> > > > > > > PATCH
>> > > > > > > 02)
>> > > > > >
>> > > > > > Instructions that take HF can only be strictly HF or mix F
>> > > > > > and
>> > > > > > HF
>> > > > > > (mixed mode float), with MOV being the only exception. That
>> > > > > > means
>> > > > > > that
>> > > > > > any instruction like the one you use above are invalid.
>> > > > > > Maybe
>> > > > > > we
>> > > > > > should
>> > > > > > validate explicitly that instructions that use HF are
>> > > > > > strictly
>> > > > > > HF
>> > > > > > or
>> > > > > > mixed-float mode only?
>> > > > > >
>> > > > >
>> > > > > So you're acknowledging that the conversions checked above
>> > > > > are
>> > > > > illegal
>> > > > > whether the instruction is a MOV or something else. Where
>> > > > > are we
>> > > > > preventing instructions other than MOV with such conversions
>> > > > > from
>> > > > > being
>> > > > > accepted by the validator?
>> > > >
>> > > > We aren't, because the validator is not checking, in general,
>> > > > for
>> > > > accepted type combinations for specific instructions anywhere
>> > > > as
>> > > > far as
>> > > > I can see.
>> > >
>> > > Luckily these type conversion restrictions aren't really specific
>> > > to
>> > > any
>>
Re: [Mesa-dev] [PATCH v4 34/40] intel/compiler: validate region restrictions for half-float conversions
On Wed, 2019-03-06 at 09:21 +0100, Iago Toral wrote:
> On Tue, 2019-03-05 at 07:35 +0100, Iago Toral wrote:
> > On Mon, 2019-03-04 at 15:36 -0800, Francisco Jerez wrote:
> > > Iago Toral writes:
> > >
> > > > On Fri, 2019-03-01 at 19:04 -0800, Francisco Jerez wrote:
> > > > > Iago Toral writes:
> > > > >
> > > > > > On Thu, 2019-02-28 at 09:54 -0800, Francisco Jerez wrote:
> > > > > > > Iago Toral writes:
> > > > > > >
> > > > > > > > On Wed, 2019-02-27 at 13:47 -0800, Francisco Jerez
> > > > > > > > wrote:
> > > > > > > > > Iago Toral writes:
> > > > > > > > >
> > > > > > > > > > On Tue, 2019-02-26 at 14:54 -0800, Francisco Jerez
> > > > > > > > > > wrote:
> > > > > > > > > > > Iago Toral Quiroga writes:
> > > > > > > > > > >
> > > > > > > > > > > > ---
> > > > > > > > > > > > src/intel/compiler/brw_eu_validate.c| 64
> > > > > > > > > > > > -
> > > > > > > > > > > > src/intel/compiler/test_eu_validate.cpp | 122
> > > > > > > > > > > >
> > > > > > > > > > > > 2 files changed, 185 insertions(+), 1
> > > > > > > > > > > > deletion(-
> > > > > > > > > > > > )
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git
> > > > > > > > > > > > a/src/intel/compiler/brw_eu_validate.c
> > > > > > > > > > > > b/src/intel/compiler/brw_eu_validate.c
> > > > > > > > > > > > index 000a05cb6ac..203641fecb9 100644
> > > > > > > > > > > > --- a/src/intel/compiler/brw_eu_validate.c
> > > > > > > > > > > > +++ b/src/intel/compiler/brw_eu_validate.c
> > > > > > > > > > > > @@ -531,7 +531,69 @@
> > > > > > > > > > > > general_restrictions_based_on_operand_types(con
> > > > > > > > > > > > st
> > > > > > > > > > > > struct
> > > > > > > > > > > > gen_device_info *devinf
> > > > > > > > > > > > exec_type_size == 8 && dst_type_size ==
> > > > > > > > > > > > 4)
> > > > > > > > > > > >dst_type_size = 8;
> > > > > > > > > > > >
> > > > > > > > > > > > - if (exec_type_size > dst_type_size) {
> > > > > > > > > > > > + /* From the BDW+ PRM:
> > > > > > > > > > > > +*
> > > > > > > > > > > > +*"There is no direct conversion from
> > > > > > > > > > > > HF
> > > > > > > > > > > > to
> > > > > > > > > > > > DF
> > > > > > > > > > > > or
> > > > > > > > > > > > DF to
> > > > > > > > > > > > HF.
> > > > > > > > > > > > +* There is no direct conversion from
> > > > > > > > > > > > HF
> > > > > > > > > > > > to
> > > > > > > > > > > > Q/UQ or
> > > > > > > > > > > > Q/UQ to
> > > > > > > > > > > > HF."
> > > > > > > > > > > > +*/
> > > > > > > > > > > > + enum brw_reg_type src0_type =
> > > > > > > > > > > > brw_inst_src0_type(devinfo,
> > > > > > > > > > > > inst);
> > > > > > > > > > > > + ERROR_IF(brw_inst_opcode(devinfo, inst) ==
> > > > > > > > > > > > BRW_OPCODE_MOV
> > > > > > > > > > > > &&
> > > > > > > > > > >
> > > > > > > > > > > Why is only the MOV instruction handled here and
> > > > > > > > > > > below? Aren't
> > > > > > > > > > > other
> > > > > > > > > > > instructions able to do implicit
> > > > > > > > > > > conversions? Probably
> > > > > > > > > > > means
> > > > > > > > > > > you
> > > > > > > > > > > need
> > > > > > > > > > > to deal with two sources rather than one.
> > > > > > > > > >
> > > > > > > > > > This comes from the programming notes of the MOV
> > > > > > > > > > instruction
> > > > > > > > > > (Volume
> > > > > > > > > > 2a, Command Reference - Instructions - MOV), so it
> > > > > > > > > > is
> > > > > > > > > > described
> > > > > > > > > > specifically for the MOV instruction. I should
> > > > > > > > > > probably
> > > > > > > > > > have
> > > > > > > > > > made
> > > > > > > > > > this
> > > > > > > > > > clear in the comment.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Maybe the one above is specified in the MOV page
> > > > > > > > > only,
> > > > > > > > > probably
> > > > > > > > > due
> > > > > > > > > to
> > > > > > > > > an oversight (If these restrictions were really
> > > > > > > > > specific
> > > > > > > > > to
> > > > > > > > > the
> > > > > > > > > MOV
> > > > > > > > > instruction, what would prevent you from implementing
> > > > > > > > > such
> > > > > > > > > conversions
> > > > > > > > > through a different instruction? E.g. "ADD dst:df,
> > > > > > > > > src:hf,
> > > > > > > > > 0"
> > > > > > > > > which
> > > > > > > > > would be substantially more efficient than what
> > > > > > > > > you're
> > > > > > > > > doing
> > > > > > > > > in
> > > > > > > > > PATCH
> > > > > > > > > 02)
> > > > > > > >
> > > > > > > > Instructions that take HF can only be strictly HF or
> > > > > > > > mix
> > > > > > > > F
> > > > > > > > and
> > > > > > > > HF
> > > > > > > > (mixed mode float), with MOV being the only exception.
> > > > > > > > That
> > > > > > > > means
> > > > > > > > that
> > > > > > > > any instruction like the one you use above are invalid.
> > > > > > > > Maybe
> > > > > > > > we
> > > > > > > > should
> > > > > > > > validate explicitly that instruct
Re: [Mesa-dev] [PATCH v4 34/40] intel/compiler: validate region restrictions for half-float conversions
On Tue, 2019-03-05 at 07:35 +0100, Iago Toral wrote:
> On Mon, 2019-03-04 at 15:36 -0800, Francisco Jerez wrote:
> > Iago Toral writes:
> >
> > > On Fri, 2019-03-01 at 19:04 -0800, Francisco Jerez wrote:
> > > > Iago Toral writes:
> > > >
> > > > > On Thu, 2019-02-28 at 09:54 -0800, Francisco Jerez wrote:
> > > > > > Iago Toral writes:
> > > > > >
> > > > > > > On Wed, 2019-02-27 at 13:47 -0800, Francisco Jerez wrote:
> > > > > > > > Iago Toral writes:
> > > > > > > >
> > > > > > > > > On Tue, 2019-02-26 at 14:54 -0800, Francisco Jerez
> > > > > > > > > wrote:
> > > > > > > > > > Iago Toral Quiroga writes:
> > > > > > > > > >
> > > > > > > > > > > ---
> > > > > > > > > > > src/intel/compiler/brw_eu_validate.c| 64
> > > > > > > > > > > -
> > > > > > > > > > > src/intel/compiler/test_eu_validate.cpp | 122
> > > > > > > > > > >
> > > > > > > > > > > 2 files changed, 185 insertions(+), 1 deletion(-
> > > > > > > > > > > )
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/src/intel/compiler/brw_eu_validate.c
> > > > > > > > > > > b/src/intel/compiler/brw_eu_validate.c
> > > > > > > > > > > index 000a05cb6ac..203641fecb9 100644
> > > > > > > > > > > --- a/src/intel/compiler/brw_eu_validate.c
> > > > > > > > > > > +++ b/src/intel/compiler/brw_eu_validate.c
> > > > > > > > > > > @@ -531,7 +531,69 @@
> > > > > > > > > > > general_restrictions_based_on_operand_types(const
> > > > > > > > > > > struct
> > > > > > > > > > > gen_device_info *devinf
> > > > > > > > > > > exec_type_size == 8 && dst_type_size ==
> > > > > > > > > > > 4)
> > > > > > > > > > >dst_type_size = 8;
> > > > > > > > > > >
> > > > > > > > > > > - if (exec_type_size > dst_type_size) {
> > > > > > > > > > > + /* From the BDW+ PRM:
> > > > > > > > > > > +*
> > > > > > > > > > > +*"There is no direct conversion from HF
> > > > > > > > > > > to
> > > > > > > > > > > DF
> > > > > > > > > > > or
> > > > > > > > > > > DF to
> > > > > > > > > > > HF.
> > > > > > > > > > > +* There is no direct conversion from HF
> > > > > > > > > > > to
> > > > > > > > > > > Q/UQ or
> > > > > > > > > > > Q/UQ to
> > > > > > > > > > > HF."
> > > > > > > > > > > +*/
> > > > > > > > > > > + enum brw_reg_type src0_type =
> > > > > > > > > > > brw_inst_src0_type(devinfo,
> > > > > > > > > > > inst);
> > > > > > > > > > > + ERROR_IF(brw_inst_opcode(devinfo, inst) ==
> > > > > > > > > > > BRW_OPCODE_MOV
> > > > > > > > > > > &&
> > > > > > > > > >
> > > > > > > > > > Why is only the MOV instruction handled here and
> > > > > > > > > > below? Aren't
> > > > > > > > > > other
> > > > > > > > > > instructions able to do implicit
> > > > > > > > > > conversions? Probably
> > > > > > > > > > means
> > > > > > > > > > you
> > > > > > > > > > need
> > > > > > > > > > to deal with two sources rather than one.
> > > > > > > > >
> > > > > > > > > This comes from the programming notes of the MOV
> > > > > > > > > instruction
> > > > > > > > > (Volume
> > > > > > > > > 2a, Command Reference - Instructions - MOV), so it is
> > > > > > > > > described
> > > > > > > > > specifically for the MOV instruction. I should
> > > > > > > > > probably
> > > > > > > > > have
> > > > > > > > > made
> > > > > > > > > this
> > > > > > > > > clear in the comment.
> > > > > > > > >
> > > > > > > >
> > > > > > > > Maybe the one above is specified in the MOV page only,
> > > > > > > > probably
> > > > > > > > due
> > > > > > > > to
> > > > > > > > an oversight (If these restrictions were really
> > > > > > > > specific
> > > > > > > > to
> > > > > > > > the
> > > > > > > > MOV
> > > > > > > > instruction, what would prevent you from implementing
> > > > > > > > such
> > > > > > > > conversions
> > > > > > > > through a different instruction? E.g. "ADD dst:df,
> > > > > > > > src:hf,
> > > > > > > > 0"
> > > > > > > > which
> > > > > > > > would be substantially more efficient than what you're
> > > > > > > > doing
> > > > > > > > in
> > > > > > > > PATCH
> > > > > > > > 02)
> > > > > > >
> > > > > > > Instructions that take HF can only be strictly HF or mix
> > > > > > > F
> > > > > > > and
> > > > > > > HF
> > > > > > > (mixed mode float), with MOV being the only exception.
> > > > > > > That
> > > > > > > means
> > > > > > > that
> > > > > > > any instruction like the one you use above are invalid.
> > > > > > > Maybe
> > > > > > > we
> > > > > > > should
> > > > > > > validate explicitly that instructions that use HF are
> > > > > > > strictly
> > > > > > > HF
> > > > > > > or
> > > > > > > mixed-float mode only?
> > > > > > >
> > > > > >
> > > > > > So you're acknowledging that the conversions checked above
> > > > > > are
> > > > > > illegal
> > > > > > whether the instruction is a MOV or something else. Where
> > > > > > are we
> > > > > > preventing instructions other than MOV with such
> > > > > > conversions
> > > > > > from
> > > > > > being
> > > > > > accepted by the validator?
Re: [Mesa-dev] [PATCH v4 34/40] intel/compiler: validate region restrictions for half-float conversions
On Mon, 2019-03-04 at 15:36 -0800, Francisco Jerez wrote:
> Iago Toral writes:
>
> > On Fri, 2019-03-01 at 19:04 -0800, Francisco Jerez wrote:
> > > Iago Toral writes:
> > >
> > > > On Thu, 2019-02-28 at 09:54 -0800, Francisco Jerez wrote:
> > > > > Iago Toral writes:
> > > > >
> > > > > > On Wed, 2019-02-27 at 13:47 -0800, Francisco Jerez wrote:
> > > > > > > Iago Toral writes:
> > > > > > >
> > > > > > > > On Tue, 2019-02-26 at 14:54 -0800, Francisco Jerez
> > > > > > > > wrote:
> > > > > > > > > Iago Toral Quiroga writes:
> > > > > > > > >
> > > > > > > > > > ---
> > > > > > > > > > src/intel/compiler/brw_eu_validate.c| 64
> > > > > > > > > > -
> > > > > > > > > > src/intel/compiler/test_eu_validate.cpp | 122
> > > > > > > > > >
> > > > > > > > > > 2 files changed, 185 insertions(+), 1 deletion(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/src/intel/compiler/brw_eu_validate.c
> > > > > > > > > > b/src/intel/compiler/brw_eu_validate.c
> > > > > > > > > > index 000a05cb6ac..203641fecb9 100644
> > > > > > > > > > --- a/src/intel/compiler/brw_eu_validate.c
> > > > > > > > > > +++ b/src/intel/compiler/brw_eu_validate.c
> > > > > > > > > > @@ -531,7 +531,69 @@
> > > > > > > > > > general_restrictions_based_on_operand_types(const
> > > > > > > > > > struct
> > > > > > > > > > gen_device_info *devinf
> > > > > > > > > > exec_type_size == 8 && dst_type_size == 4)
> > > > > > > > > >dst_type_size = 8;
> > > > > > > > > >
> > > > > > > > > > - if (exec_type_size > dst_type_size) {
> > > > > > > > > > + /* From the BDW+ PRM:
> > > > > > > > > > +*
> > > > > > > > > > +*"There is no direct conversion from HF to
> > > > > > > > > > DF
> > > > > > > > > > or
> > > > > > > > > > DF to
> > > > > > > > > > HF.
> > > > > > > > > > +* There is no direct conversion from HF to
> > > > > > > > > > Q/UQ or
> > > > > > > > > > Q/UQ to
> > > > > > > > > > HF."
> > > > > > > > > > +*/
> > > > > > > > > > + enum brw_reg_type src0_type =
> > > > > > > > > > brw_inst_src0_type(devinfo,
> > > > > > > > > > inst);
> > > > > > > > > > + ERROR_IF(brw_inst_opcode(devinfo, inst) ==
> > > > > > > > > > BRW_OPCODE_MOV
> > > > > > > > > > &&
> > > > > > > > >
> > > > > > > > > Why is only the MOV instruction handled here and
> > > > > > > > > below? Aren't
> > > > > > > > > other
> > > > > > > > > instructions able to do implicit
> > > > > > > > > conversions? Probably
> > > > > > > > > means
> > > > > > > > > you
> > > > > > > > > need
> > > > > > > > > to deal with two sources rather than one.
> > > > > > > >
> > > > > > > > This comes from the programming notes of the MOV
> > > > > > > > instruction
> > > > > > > > (Volume
> > > > > > > > 2a, Command Reference - Instructions - MOV), so it is
> > > > > > > > described
> > > > > > > > specifically for the MOV instruction. I should probably
> > > > > > > > have
> > > > > > > > made
> > > > > > > > this
> > > > > > > > clear in the comment.
> > > > > > > >
> > > > > > >
> > > > > > > Maybe the one above is specified in the MOV page only,
> > > > > > > probably
> > > > > > > due
> > > > > > > to
> > > > > > > an oversight (If these restrictions were really specific
> > > > > > > to
> > > > > > > the
> > > > > > > MOV
> > > > > > > instruction, what would prevent you from implementing
> > > > > > > such
> > > > > > > conversions
> > > > > > > through a different instruction? E.g. "ADD dst:df,
> > > > > > > src:hf,
> > > > > > > 0"
> > > > > > > which
> > > > > > > would be substantially more efficient than what you're
> > > > > > > doing
> > > > > > > in
> > > > > > > PATCH
> > > > > > > 02)
> > > > > >
> > > > > > Instructions that take HF can only be strictly HF or mix F
> > > > > > and
> > > > > > HF
> > > > > > (mixed mode float), with MOV being the only exception. That
> > > > > > means
> > > > > > that
> > > > > > any instruction like the one you use above are invalid.
> > > > > > Maybe
> > > > > > we
> > > > > > should
> > > > > > validate explicitly that instructions that use HF are
> > > > > > strictly
> > > > > > HF
> > > > > > or
> > > > > > mixed-float mode only?
> > > > > >
> > > > >
> > > > > So you're acknowledging that the conversions checked above
> > > > > are
> > > > > illegal
> > > > > whether the instruction is a MOV or something else. Where
> > > > > are we
> > > > > preventing instructions other than MOV with such conversions
> > > > > from
> > > > > being
> > > > > accepted by the validator?
> > > >
> > > > We aren't, because the validator is not checking, in general,
> > > > for
> > > > accepted type combinations for specific instructions anywhere
> > > > as
> > > > far as
> > > > I can see.
> > >
> > > Luckily these type conversion restrictions aren't really specific
> > > to
> > > any
> > > instruction AFAICT, even though they only seem to be mentioned
> > > explicitly for the MOV instruction...
> > >
> > > > If we want to start doing this
Re: [Mesa-dev] [PATCH v4 34/40] intel/compiler: validate region restrictions for half-float conversions
Iago Toral writes:
> On Fri, 2019-03-01 at 19:04 -0800, Francisco Jerez wrote:
>> Iago Toral writes:
>>
>> > On Thu, 2019-02-28 at 09:54 -0800, Francisco Jerez wrote:
>> > > Iago Toral writes:
>> > >
>> > > > On Wed, 2019-02-27 at 13:47 -0800, Francisco Jerez wrote:
>> > > > > Iago Toral writes:
>> > > > >
>> > > > > > On Tue, 2019-02-26 at 14:54 -0800, Francisco Jerez wrote:
>> > > > > > > Iago Toral Quiroga writes:
>> > > > > > >
>> > > > > > > > ---
>> > > > > > > > src/intel/compiler/brw_eu_validate.c| 64
>> > > > > > > > -
>> > > > > > > > src/intel/compiler/test_eu_validate.cpp | 122
>> > > > > > > >
>> > > > > > > > 2 files changed, 185 insertions(+), 1 deletion(-)
>> > > > > > > >
>> > > > > > > > diff --git a/src/intel/compiler/brw_eu_validate.c
>> > > > > > > > b/src/intel/compiler/brw_eu_validate.c
>> > > > > > > > index 000a05cb6ac..203641fecb9 100644
>> > > > > > > > --- a/src/intel/compiler/brw_eu_validate.c
>> > > > > > > > +++ b/src/intel/compiler/brw_eu_validate.c
>> > > > > > > > @@ -531,7 +531,69 @@
>> > > > > > > > general_restrictions_based_on_operand_types(const
>> > > > > > > > struct
>> > > > > > > > gen_device_info *devinf
>> > > > > > > > exec_type_size == 8 && dst_type_size == 4)
>> > > > > > > >dst_type_size = 8;
>> > > > > > > >
>> > > > > > > > - if (exec_type_size > dst_type_size) {
>> > > > > > > > + /* From the BDW+ PRM:
>> > > > > > > > +*
>> > > > > > > > +*"There is no direct conversion from HF to DF
>> > > > > > > > or
>> > > > > > > > DF to
>> > > > > > > > HF.
>> > > > > > > > +* There is no direct conversion from HF to
>> > > > > > > > Q/UQ or
>> > > > > > > > Q/UQ to
>> > > > > > > > HF."
>> > > > > > > > +*/
>> > > > > > > > + enum brw_reg_type src0_type =
>> > > > > > > > brw_inst_src0_type(devinfo,
>> > > > > > > > inst);
>> > > > > > > > + ERROR_IF(brw_inst_opcode(devinfo, inst) ==
>> > > > > > > > BRW_OPCODE_MOV
>> > > > > > > > &&
>> > > > > > >
>> > > > > > > Why is only the MOV instruction handled here and
>> > > > > > > below? Aren't
>> > > > > > > other
>> > > > > > > instructions able to do implicit conversions? Probably
>> > > > > > > means
>> > > > > > > you
>> > > > > > > need
>> > > > > > > to deal with two sources rather than one.
>> > > > > >
>> > > > > > This comes from the programming notes of the MOV
>> > > > > > instruction
>> > > > > > (Volume
>> > > > > > 2a, Command Reference - Instructions - MOV), so it is
>> > > > > > described
>> > > > > > specifically for the MOV instruction. I should probably
>> > > > > > have
>> > > > > > made
>> > > > > > this
>> > > > > > clear in the comment.
>> > > > > >
>> > > > >
>> > > > > Maybe the one above is specified in the MOV page only,
>> > > > > probably
>> > > > > due
>> > > > > to
>> > > > > an oversight (If these restrictions were really specific to
>> > > > > the
>> > > > > MOV
>> > > > > instruction, what would prevent you from implementing such
>> > > > > conversions
>> > > > > through a different instruction? E.g. "ADD dst:df, src:hf,
>> > > > > 0"
>> > > > > which
>> > > > > would be substantially more efficient than what you're doing
>> > > > > in
>> > > > > PATCH
>> > > > > 02)
>> > > >
>> > > > Instructions that take HF can only be strictly HF or mix F and
>> > > > HF
>> > > > (mixed mode float), with MOV being the only exception. That
>> > > > means
>> > > > that
>> > > > any instruction like the one you use above are invalid. Maybe
>> > > > we
>> > > > should
>> > > > validate explicitly that instructions that use HF are strictly
>> > > > HF
>> > > > or
>> > > > mixed-float mode only?
>> > > >
>> > >
>> > > So you're acknowledging that the conversions checked above are
>> > > illegal
>> > > whether the instruction is a MOV or something else. Where are we
>> > > preventing instructions other than MOV with such conversions from
>> > > being
>> > > accepted by the validator?
>> >
>> > We aren't, because the validator is not checking, in general, for
>> > accepted type combinations for specific instructions anywhere as
>> > far as
>> > I can see.
>>
>> Luckily these type conversion restrictions aren't really specific to
>> any
>> instruction AFAICT, even though they only seem to be mentioned
>> explicitly for the MOV instruction...
>>
>> > If we want to start doing this with HF conversions specifically, I
>> > guess it is fine, but in case it is not clear, I think it won't
>> > bring
>> > any immediate benefits with the VK_KHR_shader_float16_int8
>> > implementation since this series only ever emits conversions
>> > involving
>> > HF operands via MOV instructions,
>>
>> Yes, I can see that's the intention of this series, but how can you
>> make
>> sure that nothing in the optimizer is breaking your assumption if you
>> don't add some validator code to verify the claim of your last
>> paragraph?
>>
>> > which is why I thought that validating that no direct MOV
>> > conversions
Re: [Mesa-dev] [PATCH v4 34/40] intel/compiler: validate region restrictions for half-float conversions
On Fri, 2019-03-01 at 19:04 -0800, Francisco Jerez wrote:
> Iago Toral writes:
>
> > On Thu, 2019-02-28 at 09:54 -0800, Francisco Jerez wrote:
> > > Iago Toral writes:
> > >
> > > > On Wed, 2019-02-27 at 13:47 -0800, Francisco Jerez wrote:
> > > > > Iago Toral writes:
> > > > >
> > > > > > On Tue, 2019-02-26 at 14:54 -0800, Francisco Jerez wrote:
> > > > > > > Iago Toral Quiroga writes:
> > > > > > >
> > > > > > > > ---
> > > > > > > > src/intel/compiler/brw_eu_validate.c| 64
> > > > > > > > -
> > > > > > > > src/intel/compiler/test_eu_validate.cpp | 122
> > > > > > > >
> > > > > > > > 2 files changed, 185 insertions(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/src/intel/compiler/brw_eu_validate.c
> > > > > > > > b/src/intel/compiler/brw_eu_validate.c
> > > > > > > > index 000a05cb6ac..203641fecb9 100644
> > > > > > > > --- a/src/intel/compiler/brw_eu_validate.c
> > > > > > > > +++ b/src/intel/compiler/brw_eu_validate.c
> > > > > > > > @@ -531,7 +531,69 @@
> > > > > > > > general_restrictions_based_on_operand_types(const
> > > > > > > > struct
> > > > > > > > gen_device_info *devinf
> > > > > > > > exec_type_size == 8 && dst_type_size == 4)
> > > > > > > >dst_type_size = 8;
> > > > > > > >
> > > > > > > > - if (exec_type_size > dst_type_size) {
> > > > > > > > + /* From the BDW+ PRM:
> > > > > > > > +*
> > > > > > > > +*"There is no direct conversion from HF to DF
> > > > > > > > or
> > > > > > > > DF to
> > > > > > > > HF.
> > > > > > > > +* There is no direct conversion from HF to
> > > > > > > > Q/UQ or
> > > > > > > > Q/UQ to
> > > > > > > > HF."
> > > > > > > > +*/
> > > > > > > > + enum brw_reg_type src0_type =
> > > > > > > > brw_inst_src0_type(devinfo,
> > > > > > > > inst);
> > > > > > > > + ERROR_IF(brw_inst_opcode(devinfo, inst) ==
> > > > > > > > BRW_OPCODE_MOV
> > > > > > > > &&
> > > > > > >
> > > > > > > Why is only the MOV instruction handled here and
> > > > > > > below? Aren't
> > > > > > > other
> > > > > > > instructions able to do implicit conversions? Probably
> > > > > > > means
> > > > > > > you
> > > > > > > need
> > > > > > > to deal with two sources rather than one.
> > > > > >
> > > > > > This comes from the programming notes of the MOV
> > > > > > instruction
> > > > > > (Volume
> > > > > > 2a, Command Reference - Instructions - MOV), so it is
> > > > > > described
> > > > > > specifically for the MOV instruction. I should probably
> > > > > > have
> > > > > > made
> > > > > > this
> > > > > > clear in the comment.
> > > > > >
> > > > >
> > > > > Maybe the one above is specified in the MOV page only,
> > > > > probably
> > > > > due
> > > > > to
> > > > > an oversight (If these restrictions were really specific to
> > > > > the
> > > > > MOV
> > > > > instruction, what would prevent you from implementing such
> > > > > conversions
> > > > > through a different instruction? E.g. "ADD dst:df, src:hf,
> > > > > 0"
> > > > > which
> > > > > would be substantially more efficient than what you're doing
> > > > > in
> > > > > PATCH
> > > > > 02)
> > > >
> > > > Instructions that take HF can only be strictly HF or mix F and
> > > > HF
> > > > (mixed mode float), with MOV being the only exception. That
> > > > means
> > > > that
> > > > any instruction like the one you use above are invalid. Maybe
> > > > we
> > > > should
> > > > validate explicitly that instructions that use HF are strictly
> > > > HF
> > > > or
> > > > mixed-float mode only?
> > > >
> > >
> > > So you're acknowledging that the conversions checked above are
> > > illegal
> > > whether the instruction is a MOV or something else. Where are we
> > > preventing instructions other than MOV with such conversions from
> > > being
> > > accepted by the validator?
> >
> > We aren't, because the validator is not checking, in general, for
> > accepted type combinations for specific instructions anywhere as
> > far as
> > I can see.
>
> Luckily these type conversion restrictions aren't really specific to
> any
> instruction AFAICT, even though they only seem to be mentioned
> explicitly for the MOV instruction...
>
> > If we want to start doing this with HF conversions specifically, I
> > guess it is fine, but in case it is not clear, I think it won't
> > bring
> > any immediate benefits with the VK_KHR_shader_float16_int8
> > implementation since this series only ever emits conversions
> > involving
> > HF operands via MOV instructions,
>
> Yes, I can see that's the intention of this series, but how can you
> make
> sure that nothing in the optimizer is breaking your assumption if you
> don't add some validator code to verify the claim of your last
> paragraph?
>
> > which is why I thought that validating that no direct MOV
> > conversions
> > from DF/Q types ever happen was useful, since we have code in the
> > driver to handle this scenario.
> >
> > [...]
> > >
> > > I still don't understa
Re: [Mesa-dev] [PATCH v4 34/40] intel/compiler: validate region restrictions for half-float conversions
Iago Toral writes:
> On Fri, 2019-03-01 at 09:39 +0100, Iago Toral wrote:
>> On Thu, 2019-02-28 at 09:54 -0800, Francisco Jerez wrote:
>> > Iago Toral writes:
>> >
>> > > On Wed, 2019-02-27 at 13:47 -0800, Francisco Jerez wrote:
>> > > > Iago Toral writes:
>> > > >
>> > > > > On Tue, 2019-02-26 at 14:54 -0800, Francisco Jerez wrote:
>> > > > > > Iago Toral Quiroga writes:
>> > > > > >
>> > > > > > > ---
>> > > > > > > src/intel/compiler/brw_eu_validate.c| 64
>> > > > > > > -
>> > > > > > > src/intel/compiler/test_eu_validate.cpp | 122
>> > > > > > >
>> > > > > > > 2 files changed, 185 insertions(+), 1 deletion(-)
>> > > > > > >
>> > > > > > > diff --git a/src/intel/compiler/brw_eu_validate.c
>> > > > > > > b/src/intel/compiler/brw_eu_validate.c
>> > > > > > > index 000a05cb6ac..203641fecb9 100644
>> > > > > > > --- a/src/intel/compiler/brw_eu_validate.c
>> > > > > > > +++ b/src/intel/compiler/brw_eu_validate.c
>> > > > > > > @@ -531,7 +531,69 @@
>> > > > > > > general_restrictions_based_on_operand_types(const struct
>> > > > > > > gen_device_info *devinf
>> > > > > > > exec_type_size == 8 && dst_type_size == 4)
>> > > > > > >dst_type_size = 8;
>> > > > > > >
>> > > > > > > - if (exec_type_size > dst_type_size) {
>> > > > > > > + /* From the BDW+ PRM:
>> > > > > > > +*
>> > > > > > > +*"There is no direct conversion from HF to DF or
>> > > > > > > DF to
>> > > > > > > HF.
>> > > > > > > +* There is no direct conversion from HF to Q/UQ
>> > > > > > > or
>> > > > > > > Q/UQ to
>> > > > > > > HF."
>> > > > > > > +*/
>> > > > > > > + enum brw_reg_type src0_type =
>> > > > > > > brw_inst_src0_type(devinfo,
>> > > > > > > inst);
>> > > > > > > + ERROR_IF(brw_inst_opcode(devinfo, inst) ==
>> > > > > > > BRW_OPCODE_MOV
>> > > > > > > &&
>> > > > > >
>> > > > > > Why is only the MOV instruction handled here and
>> > > > > > below? Aren't
>> > > > > > other
>> > > > > > instructions able to do implicit conversions? Probably
>> > > > > > means
>> > > > > > you
>> > > > > > need
>> > > > > > to deal with two sources rather than one.
>> > > > >
>> > > > > This comes from the programming notes of the MOV instruction
>> > > > > (Volume
>> > > > > 2a, Command Reference - Instructions - MOV), so it is
>> > > > > described
>> > > > > specifically for the MOV instruction. I should probably have
>> > > > > made
>> > > > > this
>> > > > > clear in the comment.
>> > > > >
>> > > >
>> > > > Maybe the one above is specified in the MOV page only, probably
>> > > > due
>> > > > to
>> > > > an oversight (If these restrictions were really specific to the
>> > > > MOV
>> > > > instruction, what would prevent you from implementing such
>> > > > conversions
>> > > > through a different instruction? E.g. "ADD dst:df, src:hf, 0"
>> > > > which
>> > > > would be substantially more efficient than what you're doing in
>> > > > PATCH
>> > > > 02)
>> > >
>> > > Instructions that take HF can only be strictly HF or mix F and HF
>> > > (mixed mode float), with MOV being the only exception. That means
>> > > that
>> > > any instruction like the one you use above are invalid. Maybe we
>> > > should
>> > > validate explicitly that instructions that use HF are strictly HF
>> > > or
>> > > mixed-float mode only?
>> > >
>> >
>> > So you're acknowledging that the conversions checked above are
>> > illegal
>> > whether the instruction is a MOV or something else. Where are we
>> > preventing instructions other than MOV with such conversions from
>> > being
>> > accepted by the validator?
>>
>> We aren't, because the validator is not checking, in general, for
>> accepted type combinations for specific instructions anywhere as far
>> as
>> I can see. If we want to start doing this with HF conversions
>> specifically, I guess it is fine, but in case it is not clear, I
>> think
>> it won't bring any immediate benefits with the
>> VK_KHR_shader_float16_int8 implementation since this series only ever
>> emits conversions involving HF operands via MOV instructions, which
>> is
>> why I thought that validating that no direct MOV conversions from
>> DF/Q
>> types ever happen was useful, since we have code in the driver to
>> handle this scenario.
>>
>> > > > but I see other restriction checks in this patch which are
>> > > > certainly specified in the generic regioning restrictions page
>> > > > and
>> > > > you're limiting to the MOV instruction...
>> > >
>> > > There are two rules below:
>> > >
>> > > 1. The one about conversions between integer and half-float.
>> > > Again,
>> > > these can only happen through MOV for the same reasons, so I
>> > > think
>> > > this
>> > > one should be fine.
>> > >
>> >
>> > Why do you think that can only happen through a MOV
>> > instruction? The
>> > hardware spec lists the following as a valid example in the
>> > register
>> > region restrictions page:
>> >
>> > > add (8) r10.0<2>:hf r11.0<8;8,1>:w r12.0
Re: [Mesa-dev] [PATCH v4 34/40] intel/compiler: validate region restrictions for half-float conversions
Iago Toral writes:
> On Thu, 2019-02-28 at 09:54 -0800, Francisco Jerez wrote:
>> Iago Toral writes:
>>
>> > On Wed, 2019-02-27 at 13:47 -0800, Francisco Jerez wrote:
>> > > Iago Toral writes:
>> > >
>> > > > On Tue, 2019-02-26 at 14:54 -0800, Francisco Jerez wrote:
>> > > > > Iago Toral Quiroga writes:
>> > > > >
>> > > > > > ---
>> > > > > > src/intel/compiler/brw_eu_validate.c| 64
>> > > > > > -
>> > > > > > src/intel/compiler/test_eu_validate.cpp | 122
>> > > > > >
>> > > > > > 2 files changed, 185 insertions(+), 1 deletion(-)
>> > > > > >
>> > > > > > diff --git a/src/intel/compiler/brw_eu_validate.c
>> > > > > > b/src/intel/compiler/brw_eu_validate.c
>> > > > > > index 000a05cb6ac..203641fecb9 100644
>> > > > > > --- a/src/intel/compiler/brw_eu_validate.c
>> > > > > > +++ b/src/intel/compiler/brw_eu_validate.c
>> > > > > > @@ -531,7 +531,69 @@
>> > > > > > general_restrictions_based_on_operand_types(const struct
>> > > > > > gen_device_info *devinf
>> > > > > > exec_type_size == 8 && dst_type_size == 4)
>> > > > > >dst_type_size = 8;
>> > > > > >
>> > > > > > - if (exec_type_size > dst_type_size) {
>> > > > > > + /* From the BDW+ PRM:
>> > > > > > +*
>> > > > > > +*"There is no direct conversion from HF to DF or
>> > > > > > DF to
>> > > > > > HF.
>> > > > > > +* There is no direct conversion from HF to Q/UQ or
>> > > > > > Q/UQ to
>> > > > > > HF."
>> > > > > > +*/
>> > > > > > + enum brw_reg_type src0_type =
>> > > > > > brw_inst_src0_type(devinfo,
>> > > > > > inst);
>> > > > > > + ERROR_IF(brw_inst_opcode(devinfo, inst) ==
>> > > > > > BRW_OPCODE_MOV
>> > > > > > &&
>> > > > >
>> > > > > Why is only the MOV instruction handled here and
>> > > > > below? Aren't
>> > > > > other
>> > > > > instructions able to do implicit conversions? Probably means
>> > > > > you
>> > > > > need
>> > > > > to deal with two sources rather than one.
>> > > >
>> > > > This comes from the programming notes of the MOV instruction
>> > > > (Volume
>> > > > 2a, Command Reference - Instructions - MOV), so it is described
>> > > > specifically for the MOV instruction. I should probably have
>> > > > made
>> > > > this
>> > > > clear in the comment.
>> > > >
>> > >
>> > > Maybe the one above is specified in the MOV page only, probably
>> > > due
>> > > to
>> > > an oversight (If these restrictions were really specific to the
>> > > MOV
>> > > instruction, what would prevent you from implementing such
>> > > conversions
>> > > through a different instruction? E.g. "ADD dst:df, src:hf, 0"
>> > > which
>> > > would be substantially more efficient than what you're doing in
>> > > PATCH
>> > > 02)
>> >
>> > Instructions that take HF can only be strictly HF or mix F and HF
>> > (mixed mode float), with MOV being the only exception. That means
>> > that
>> > any instruction like the one you use above are invalid. Maybe we
>> > should
>> > validate explicitly that instructions that use HF are strictly HF
>> > or
>> > mixed-float mode only?
>> >
>>
>> So you're acknowledging that the conversions checked above are
>> illegal
>> whether the instruction is a MOV or something else. Where are we
>> preventing instructions other than MOV with such conversions from
>> being
>> accepted by the validator?
>
> We aren't, because the validator is not checking, in general, for
> accepted type combinations for specific instructions anywhere as far as
> I can see.
Luckily these type conversion restrictions aren't really specific to any
instruction AFAICT, even though they only seem to be mentioned
explicitly for the MOV instruction...
> If we want to start doing this with HF conversions specifically, I
> guess it is fine, but in case it is not clear, I think it won't bring
> any immediate benefits with the VK_KHR_shader_float16_int8
> implementation since this series only ever emits conversions involving
> HF operands via MOV instructions,
Yes, I can see that's the intention of this series, but how can you make
sure that nothing in the optimizer is breaking your assumption if you
don't add some validator code to verify the claim of your last
paragraph?
> which is why I thought that validating that no direct MOV conversions
> from DF/Q types ever happen was useful, since we have code in the
> driver to handle this scenario.
>
>[...]
>>
>> I still don't understand why you want to implement the same
>> restriction
>> twice, once for MOV and once for all other mixed-mode
>> instructions. How
>> is that more convenient?
>
> The restrictions based on operand types that are checked in the
> validator are specific to Byte or cases where the execution type is
> larger than the destination stride, for which mixed float has a
> different set of restrictions.
>
> For example, for mixed float we have this rule:
>
> "In Align1, destination stride can be smaller than execution type"
>
> Which overrides this other from "General restrictions ba
Re: [Mesa-dev] [PATCH v4 34/40] intel/compiler: validate region restrictions for half-float conversions
On Fri, 2019-03-01 at 09:39 +0100, Iago Toral wrote:
> On Thu, 2019-02-28 at 09:54 -0800, Francisco Jerez wrote:
> > Iago Toral writes:
> >
> > > On Wed, 2019-02-27 at 13:47 -0800, Francisco Jerez wrote:
> > > > Iago Toral writes:
> > > >
> > > > > On Tue, 2019-02-26 at 14:54 -0800, Francisco Jerez wrote:
> > > > > > Iago Toral Quiroga writes:
> > > > > >
> > > > > > > ---
> > > > > > > src/intel/compiler/brw_eu_validate.c| 64
> > > > > > > -
> > > > > > > src/intel/compiler/test_eu_validate.cpp | 122
> > > > > > >
> > > > > > > 2 files changed, 185 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/src/intel/compiler/brw_eu_validate.c
> > > > > > > b/src/intel/compiler/brw_eu_validate.c
> > > > > > > index 000a05cb6ac..203641fecb9 100644
> > > > > > > --- a/src/intel/compiler/brw_eu_validate.c
> > > > > > > +++ b/src/intel/compiler/brw_eu_validate.c
> > > > > > > @@ -531,7 +531,69 @@
> > > > > > > general_restrictions_based_on_operand_types(const struct
> > > > > > > gen_device_info *devinf
> > > > > > > exec_type_size == 8 && dst_type_size == 4)
> > > > > > >dst_type_size = 8;
> > > > > > >
> > > > > > > - if (exec_type_size > dst_type_size) {
> > > > > > > + /* From the BDW+ PRM:
> > > > > > > +*
> > > > > > > +*"There is no direct conversion from HF to DF or
> > > > > > > DF to
> > > > > > > HF.
> > > > > > > +* There is no direct conversion from HF to Q/UQ
> > > > > > > or
> > > > > > > Q/UQ to
> > > > > > > HF."
> > > > > > > +*/
> > > > > > > + enum brw_reg_type src0_type =
> > > > > > > brw_inst_src0_type(devinfo,
> > > > > > > inst);
> > > > > > > + ERROR_IF(brw_inst_opcode(devinfo, inst) ==
> > > > > > > BRW_OPCODE_MOV
> > > > > > > &&
> > > > > >
> > > > > > Why is only the MOV instruction handled here and
> > > > > > below? Aren't
> > > > > > other
> > > > > > instructions able to do implicit conversions? Probably
> > > > > > means
> > > > > > you
> > > > > > need
> > > > > > to deal with two sources rather than one.
> > > > >
> > > > > This comes from the programming notes of the MOV instruction
> > > > > (Volume
> > > > > 2a, Command Reference - Instructions - MOV), so it is
> > > > > described
> > > > > specifically for the MOV instruction. I should probably have
> > > > > made
> > > > > this
> > > > > clear in the comment.
> > > > >
> > > >
> > > > Maybe the one above is specified in the MOV page only, probably
> > > > due
> > > > to
> > > > an oversight (If these restrictions were really specific to the
> > > > MOV
> > > > instruction, what would prevent you from implementing such
> > > > conversions
> > > > through a different instruction? E.g. "ADD dst:df, src:hf, 0"
> > > > which
> > > > would be substantially more efficient than what you're doing in
> > > > PATCH
> > > > 02)
> > >
> > > Instructions that take HF can only be strictly HF or mix F and HF
> > > (mixed mode float), with MOV being the only exception. That means
> > > that
> > > any instruction like the one you use above are invalid. Maybe we
> > > should
> > > validate explicitly that instructions that use HF are strictly HF
> > > or
> > > mixed-float mode only?
> > >
> >
> > So you're acknowledging that the conversions checked above are
> > illegal
> > whether the instruction is a MOV or something else. Where are we
> > preventing instructions other than MOV with such conversions from
> > being
> > accepted by the validator?
>
> We aren't, because the validator is not checking, in general, for
> accepted type combinations for specific instructions anywhere as far
> as
> I can see. If we want to start doing this with HF conversions
> specifically, I guess it is fine, but in case it is not clear, I
> think
> it won't bring any immediate benefits with the
> VK_KHR_shader_float16_int8 implementation since this series only ever
> emits conversions involving HF operands via MOV instructions, which
> is
> why I thought that validating that no direct MOV conversions from
> DF/Q
> types ever happen was useful, since we have code in the driver to
> handle this scenario.
>
> > > > but I see other restriction checks in this patch which are
> > > > certainly specified in the generic regioning restrictions page
> > > > and
> > > > you're limiting to the MOV instruction...
> > >
> > > There are two rules below:
> > >
> > > 1. The one about conversions between integer and half-float.
> > > Again,
> > > these can only happen through MOV for the same reasons, so I
> > > think
> > > this
> > > one should be fine.
> > >
> >
> > Why do you think that can only happen through a MOV
> > instruction? The
> > hardware spec lists the following as a valid example in the
> > register
> > region restrictions page:
> >
> > > add (8) r10.0<2>:hf r11.0<8;8,1>:w r12.0<8;8,1>:w
>
> I thought these instructions were not valid. I looked again at the
> BDW
> PRM and indeed, it lists that HF destination for ADD is only all
Re: [Mesa-dev] [PATCH v4 34/40] intel/compiler: validate region restrictions for half-float conversions
On Thu, 2019-02-28 at 09:54 -0800, Francisco Jerez wrote:
> Iago Toral writes:
>
> > On Wed, 2019-02-27 at 13:47 -0800, Francisco Jerez wrote:
> > > Iago Toral writes:
> > >
> > > > On Tue, 2019-02-26 at 14:54 -0800, Francisco Jerez wrote:
> > > > > Iago Toral Quiroga writes:
> > > > >
> > > > > > ---
> > > > > > src/intel/compiler/brw_eu_validate.c| 64
> > > > > > -
> > > > > > src/intel/compiler/test_eu_validate.cpp | 122
> > > > > >
> > > > > > 2 files changed, 185 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/src/intel/compiler/brw_eu_validate.c
> > > > > > b/src/intel/compiler/brw_eu_validate.c
> > > > > > index 000a05cb6ac..203641fecb9 100644
> > > > > > --- a/src/intel/compiler/brw_eu_validate.c
> > > > > > +++ b/src/intel/compiler/brw_eu_validate.c
> > > > > > @@ -531,7 +531,69 @@
> > > > > > general_restrictions_based_on_operand_types(const struct
> > > > > > gen_device_info *devinf
> > > > > > exec_type_size == 8 && dst_type_size == 4)
> > > > > >dst_type_size = 8;
> > > > > >
> > > > > > - if (exec_type_size > dst_type_size) {
> > > > > > + /* From the BDW+ PRM:
> > > > > > +*
> > > > > > +*"There is no direct conversion from HF to DF or
> > > > > > DF to
> > > > > > HF.
> > > > > > +* There is no direct conversion from HF to Q/UQ or
> > > > > > Q/UQ to
> > > > > > HF."
> > > > > > +*/
> > > > > > + enum brw_reg_type src0_type =
> > > > > > brw_inst_src0_type(devinfo,
> > > > > > inst);
> > > > > > + ERROR_IF(brw_inst_opcode(devinfo, inst) ==
> > > > > > BRW_OPCODE_MOV
> > > > > > &&
> > > > >
> > > > > Why is only the MOV instruction handled here and
> > > > > below? Aren't
> > > > > other
> > > > > instructions able to do implicit conversions? Probably means
> > > > > you
> > > > > need
> > > > > to deal with two sources rather than one.
> > > >
> > > > This comes from the programming notes of the MOV instruction
> > > > (Volume
> > > > 2a, Command Reference - Instructions - MOV), so it is described
> > > > specifically for the MOV instruction. I should probably have
> > > > made
> > > > this
> > > > clear in the comment.
> > > >
> > >
> > > Maybe the one above is specified in the MOV page only, probably
> > > due
> > > to
> > > an oversight (If these restrictions were really specific to the
> > > MOV
> > > instruction, what would prevent you from implementing such
> > > conversions
> > > through a different instruction? E.g. "ADD dst:df, src:hf, 0"
> > > which
> > > would be substantially more efficient than what you're doing in
> > > PATCH
> > > 02)
> >
> > Instructions that take HF can only be strictly HF or mix F and HF
> > (mixed mode float), with MOV being the only exception. That means
> > that
> > any instruction like the one you use above are invalid. Maybe we
> > should
> > validate explicitly that instructions that use HF are strictly HF
> > or
> > mixed-float mode only?
> >
>
> So you're acknowledging that the conversions checked above are
> illegal
> whether the instruction is a MOV or something else. Where are we
> preventing instructions other than MOV with such conversions from
> being
> accepted by the validator?
We aren't, because the validator is not checking, in general, for
accepted type combinations for specific instructions anywhere as far as
I can see. If we want to start doing this with HF conversions
specifically, I guess it is fine, but in case it is not clear, I think
it won't bring any immediate benefits with the
VK_KHR_shader_float16_int8 implementation since this series only ever
emits conversions involving HF operands via MOV instructions, which is
why I thought that validating that no direct MOV conversions from DF/Q
types ever happen was useful, since we have code in the driver to
handle this scenario.
> > > but I see other restriction checks in this patch which are
> > > certainly specified in the generic regioning restrictions page
> > > and
> > > you're limiting to the MOV instruction...
> >
> > There are two rules below:
> >
> > 1. The one about conversions between integer and half-float. Again,
> > these can only happen through MOV for the same reasons, so I think
> > this
> > one should be fine.
> >
>
> Why do you think that can only happen through a MOV instruction? The
> hardware spec lists the following as a valid example in the register
> region restrictions page:
>
> > add (8) r10.0<2>:hf r11.0<8;8,1>:w r12.0<8;8,1>:w
I thought these instructions were not valid. I looked again at the BDW
PRM and indeed, it lists that HF destination for ADD is only allowed
with HF source. The same for all other instructions that can take HF
except (S)MOV. However, I have just now realized that the SKL+ PRM is
different and it allows ADD to do this, but as far as I can see, ADD is
the only exception: you cannot do this with MUL, MAD or MATH for
example. I am not sure what to make of this, but I guess that if we
want to validate
Re: [Mesa-dev] [PATCH v4 34/40] intel/compiler: validate region restrictions for half-float conversions
Iago Toral writes:
> On Wed, 2019-02-27 at 13:47 -0800, Francisco Jerez wrote:
>> Iago Toral writes:
>>
>> > On Tue, 2019-02-26 at 14:54 -0800, Francisco Jerez wrote:
>> > > Iago Toral Quiroga writes:
>> > >
>> > > > ---
>> > > > src/intel/compiler/brw_eu_validate.c| 64 -
>> > > > src/intel/compiler/test_eu_validate.cpp | 122
>> > > >
>> > > > 2 files changed, 185 insertions(+), 1 deletion(-)
>> > > >
>> > > > diff --git a/src/intel/compiler/brw_eu_validate.c
>> > > > b/src/intel/compiler/brw_eu_validate.c
>> > > > index 000a05cb6ac..203641fecb9 100644
>> > > > --- a/src/intel/compiler/brw_eu_validate.c
>> > > > +++ b/src/intel/compiler/brw_eu_validate.c
>> > > > @@ -531,7 +531,69 @@
>> > > > general_restrictions_based_on_operand_types(const struct
>> > > > gen_device_info *devinf
>> > > > exec_type_size == 8 && dst_type_size == 4)
>> > > >dst_type_size = 8;
>> > > >
>> > > > - if (exec_type_size > dst_type_size) {
>> > > > + /* From the BDW+ PRM:
>> > > > +*
>> > > > +*"There is no direct conversion from HF to DF or DF to
>> > > > HF.
>> > > > +* There is no direct conversion from HF to Q/UQ or
>> > > > Q/UQ to
>> > > > HF."
>> > > > +*/
>> > > > + enum brw_reg_type src0_type = brw_inst_src0_type(devinfo,
>> > > > inst);
>> > > > + ERROR_IF(brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV
>> > > > &&
>> > >
>> > > Why is only the MOV instruction handled here and below? Aren't
>> > > other
>> > > instructions able to do implicit conversions? Probably means you
>> > > need
>> > > to deal with two sources rather than one.
>> >
>> > This comes from the programming notes of the MOV instruction
>> > (Volume
>> > 2a, Command Reference - Instructions - MOV), so it is described
>> > specifically for the MOV instruction. I should probably have made
>> > this
>> > clear in the comment.
>> >
>>
>> Maybe the one above is specified in the MOV page only, probably due
>> to
>> an oversight (If these restrictions were really specific to the MOV
>> instruction, what would prevent you from implementing such
>> conversions
>> through a different instruction? E.g. "ADD dst:df, src:hf, 0" which
>> would be substantially more efficient than what you're doing in PATCH
>> 02)
>
> Instructions that take HF can only be strictly HF or mix F and HF
> (mixed mode float), with MOV being the only exception. That means that
> any instruction like the one you use above are invalid. Maybe we should
> validate explicitly that instructions that use HF are strictly HF or
> mixed-float mode only?
>
So you're acknowledging that the conversions checked above are illegal
whether the instruction is a MOV or something else. Where are we
preventing instructions other than MOV with such conversions from being
accepted by the validator?
>> but I see other restriction checks in this patch which are
>> certainly specified in the generic regioning restrictions page and
>> you're limiting to the MOV instruction...
>
> There are two rules below:
>
> 1. The one about conversions between integer and half-float. Again,
> these can only happen through MOV for the same reasons, so I think this
> one should be fine.
>
Why do you think that can only happen through a MOV instruction? The
hardware spec lists the following as a valid example in the register
region restrictions page:
| add (8) r10.0<2>:hf r11.0<8;8,1>:w r12.0<8;8,1>:w
> 2. The one about word destinations (of which we are only really
> implementing conversions from F->HF). Here the rule is more generic and
> I agree that expanding this to include any other mixed float mode
> instruction would make sense. However, validation for mixed float mode
> has its own set rules, some of which are incompatible with the general
> region restrictions being validated here, so I think it is inconvenient
> to try and do that validation here (see patch 36 and then patch 37).
> What I would propose, if you agree, is that we only implement this for
> MOV here, and then for mixed float mode instructions, we implement the
> more generic version of this check (that would then go in patch 37).
> How does that sound?
>
I still don't understand why you want to implement the same restriction
twice, once for MOV and once for all other mixed-mode instructions. How
is that more convenient?
>> > > > +((dst_type == BRW_REGISTER_TYPE_HF &&
>> > > > type_sz(src0_type) == 8) ||
>> > > > + (dst_type_size == 8 && src0_type ==
>> > > > BRW_REGISTER_TYPE_HF)),
>> > > > +"There are no direct conversion between 64-bit
>> > > > types
>> > > > and HF");
>> > > > +
>> > > > + /* From the BDW+ PRM:
>> > > > +*
>> > > > +* "Conversion between Integer and HF (Half Float) must
>> > > > be
>> > > > +*DWord-aligned and strided by a DWord on the
>> > > > destination."
>> > > > +*
>> > > > +* But this seems to be expanded on CHV and SKL+ by:
>> > > > +*
>> > > > +
Re: [Mesa-dev] [PATCH v4 34/40] intel/compiler: validate region restrictions for half-float conversions
On Wed, 2019-02-27 at 13:47 -0800, Francisco Jerez wrote:
> Iago Toral writes:
>
> > On Tue, 2019-02-26 at 14:54 -0800, Francisco Jerez wrote:
> > > Iago Toral Quiroga writes:
> > >
> > > > ---
> > > > src/intel/compiler/brw_eu_validate.c| 64 -
> > > > src/intel/compiler/test_eu_validate.cpp | 122
> > > >
> > > > 2 files changed, 185 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/intel/compiler/brw_eu_validate.c
> > > > b/src/intel/compiler/brw_eu_validate.c
> > > > index 000a05cb6ac..203641fecb9 100644
> > > > --- a/src/intel/compiler/brw_eu_validate.c
> > > > +++ b/src/intel/compiler/brw_eu_validate.c
> > > > @@ -531,7 +531,69 @@
> > > > general_restrictions_based_on_operand_types(const struct
> > > > gen_device_info *devinf
> > > > exec_type_size == 8 && dst_type_size == 4)
> > > >dst_type_size = 8;
> > > >
> > > > - if (exec_type_size > dst_type_size) {
> > > > + /* From the BDW+ PRM:
> > > > +*
> > > > +*"There is no direct conversion from HF to DF or DF to
> > > > HF.
> > > > +* There is no direct conversion from HF to Q/UQ or
> > > > Q/UQ to
> > > > HF."
> > > > +*/
> > > > + enum brw_reg_type src0_type = brw_inst_src0_type(devinfo,
> > > > inst);
> > > > + ERROR_IF(brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV
> > > > &&
> > >
> > > Why is only the MOV instruction handled here and below? Aren't
> > > other
> > > instructions able to do implicit conversions? Probably means you
> > > need
> > > to deal with two sources rather than one.
> >
> > This comes from the programming notes of the MOV instruction
> > (Volume
> > 2a, Command Reference - Instructions - MOV), so it is described
> > specifically for the MOV instruction. I should probably have made
> > this
> > clear in the comment.
> >
>
> Maybe the one above is specified in the MOV page only, probably due
> to
> an oversight (If these restrictions were really specific to the MOV
> instruction, what would prevent you from implementing such
> conversions
> through a different instruction? E.g. "ADD dst:df, src:hf, 0" which
> would be substantially more efficient than what you're doing in PATCH
> 02)
Instructions that take HF can only be strictly HF or mix F and HF
(mixed mode float), with MOV being the only exception. That means that
any instruction like the one you use above are invalid. Maybe we should
validate explicitly that instructions that use HF are strictly HF or
mixed-float mode only?
> but I see other restriction checks in this patch which are
> certainly specified in the generic regioning restrictions page and
> you're limiting to the MOV instruction...
There are two rules below:
1. The one about conversions between integer and half-float. Again,
these can only happen through MOV for the same reasons, so I think this
one should be fine.
2. The one about word destinations (of which we are only really
implementing conversions from F->HF). Here the rule is more generic and
I agree that expanding this to include any other mixed float mode
instruction would make sense. However, validation for mixed float mode
has its own set rules, some of which are incompatible with the general
region restrictions being validated here, so I think it is inconvenient
to try and do that validation here (see patch 36 and then patch 37).
What I would propose, if you agree, is that we only implement this for
MOV here, and then for mixed float mode instructions, we implement the
more generic version of this check (that would then go in patch 37).
How does that sound?
> > > > +((dst_type == BRW_REGISTER_TYPE_HF &&
> > > > type_sz(src0_type) == 8) ||
> > > > + (dst_type_size == 8 && src0_type ==
> > > > BRW_REGISTER_TYPE_HF)),
> > > > +"There are no direct conversion between 64-bit
> > > > types
> > > > and HF");
> > > > +
> > > > + /* From the BDW+ PRM:
> > > > +*
> > > > +* "Conversion between Integer and HF (Half Float) must
> > > > be
> > > > +*DWord-aligned and strided by a DWord on the
> > > > destination."
> > > > +*
> > > > +* But this seems to be expanded on CHV and SKL+ by:
> > > > +*
> > > > +* "There is a relaxed alignment rule for word
> > > > destinations.
> > > > When
> > > > +*the destination type is word (UW, W, HF), destination
> > > > data types
> > > > +*can be aligned to either the lowest word or the
> > > > second
> > > > lowest
> > > > +*word of the execution channel. This means the
> > > > destination
> > > > data
> > > > +*words can be either all in the even word locations or
> > > > all
> > > > in the
> > > > +*odd word locations."
> > > > +*
> > > > +* We do not implement the second rule as is though, since
> > > > empirical testing
> > > > +* shows inconsistencies:
> > > > +* - It suggests that packed 16-bit is not allowed, which
> > > > is
> > > > not true.
> > > > +* - I
Re: [Mesa-dev] [PATCH v4 34/40] intel/compiler: validate region restrictions for half-float conversions
Iago Toral writes:
> On Tue, 2019-02-26 at 14:54 -0800, Francisco Jerez wrote:
>> Iago Toral Quiroga writes:
>>
>> > ---
>> > src/intel/compiler/brw_eu_validate.c| 64 -
>> > src/intel/compiler/test_eu_validate.cpp | 122
>> >
>> > 2 files changed, 185 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/src/intel/compiler/brw_eu_validate.c
>> > b/src/intel/compiler/brw_eu_validate.c
>> > index 000a05cb6ac..203641fecb9 100644
>> > --- a/src/intel/compiler/brw_eu_validate.c
>> > +++ b/src/intel/compiler/brw_eu_validate.c
>> > @@ -531,7 +531,69 @@
>> > general_restrictions_based_on_operand_types(const struct
>> > gen_device_info *devinf
>> > exec_type_size == 8 && dst_type_size == 4)
>> >dst_type_size = 8;
>> >
>> > - if (exec_type_size > dst_type_size) {
>> > + /* From the BDW+ PRM:
>> > +*
>> > +*"There is no direct conversion from HF to DF or DF to HF.
>> > +* There is no direct conversion from HF to Q/UQ or Q/UQ to
>> > HF."
>> > +*/
>> > + enum brw_reg_type src0_type = brw_inst_src0_type(devinfo,
>> > inst);
>> > + ERROR_IF(brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV &&
>>
>> Why is only the MOV instruction handled here and below? Aren't other
>> instructions able to do implicit conversions? Probably means you
>> need
>> to deal with two sources rather than one.
>
> This comes from the programming notes of the MOV instruction (Volume
> 2a, Command Reference - Instructions - MOV), so it is described
> specifically for the MOV instruction. I should probably have made this
> clear in the comment.
>
Maybe the one above is specified in the MOV page only, probably due to
an oversight (If these restrictions were really specific to the MOV
instruction, what would prevent you from implementing such conversions
through a different instruction? E.g. "ADD dst:df, src:hf, 0" which
would be substantially more efficient than what you're doing in PATCH
02), but I see other restriction checks in this patch which are
certainly specified in the generic regioning restrictions page and
you're limiting to the MOV instruction...
>> > +((dst_type == BRW_REGISTER_TYPE_HF &&
>> > type_sz(src0_type) == 8) ||
>> > + (dst_type_size == 8 && src0_type ==
>> > BRW_REGISTER_TYPE_HF)),
>> > +"There are no direct conversion between 64-bit types
>> > and HF");
>> > +
>> > + /* From the BDW+ PRM:
>> > +*
>> > +* "Conversion between Integer and HF (Half Float) must be
>> > +*DWord-aligned and strided by a DWord on the destination."
>> > +*
>> > +* But this seems to be expanded on CHV and SKL+ by:
>> > +*
>> > +* "There is a relaxed alignment rule for word destinations.
>> > When
>> > +*the destination type is word (UW, W, HF), destination
>> > data types
>> > +*can be aligned to either the lowest word or the second
>> > lowest
>> > +*word of the execution channel. This means the destination
>> > data
>> > +*words can be either all in the even word locations or all
>> > in the
>> > +*odd word locations."
>> > +*
>> > +* We do not implement the second rule as is though, since
>> > empirical testing
>> > +* shows inconsistencies:
>> > +* - It suggests that packed 16-bit is not allowed, which is
>> > not true.
>> > +* - It suggests that conversions from Q/DF to W (which need
>> > to be 64-bit
>> > +* aligned on the destination) are not possible, which is
>> > not true.
>> > +* - It suggests that conversions from 16-bit executions
>> > types to W need
>> > +* to be 32-bit aligned, which doesn't seem to be
>> > necessary.
>> > +*
>> > +* So from this rule we only validate the implication that
>> > conversion from
>> > +* F to HF needs to be DWord aligned too (in BDW this is
>> > limited to
>> > +* conversions from integer types).
>> > +*/
>> > + bool is_half_float_conversion =
>> > + brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV &&
>> > + dst_type != src0_type &&
>> > + (dst_type == BRW_REGISTER_TYPE_HF || src0_type ==
>> > BRW_REGISTER_TYPE_HF);
>> > +
>> > + if (is_half_float_conversion) {
>> > + assert(devinfo->gen >= 8);
>> > +
>> > + if ((dst_type == BRW_REGISTER_TYPE_HF &&
>> > brw_reg_type_is_integer(src0_type)) ||
>> > + (brw_reg_type_is_integer(dst_type) && src0_type ==
>> > BRW_REGISTER_TYPE_HF)) {
>> > + ERROR_IF(dst_stride * dst_type_size != 4,
>> > + "Conversions between integer and half-float must
>> > be strided "
>> > + "by a DWord on the destination");
>> > +
>> > + unsigned subreg = brw_inst_dst_da1_subreg_nr(devinfo,
>> > inst);
>> > + ERROR_IF(subreg % 4 != 0,
>> > + "Conversions between integer and half-float must
>> > be aligned "
>> > + "to a DWord on the destination");
>> > + } else if ((devinfo->is_ch
Re: [Mesa-dev] [PATCH v4 34/40] intel/compiler: validate region restrictions for half-float conversions
On Tue, 2019-02-26 at 14:54 -0800, Francisco Jerez wrote:
> Iago Toral Quiroga writes:
>
> > ---
> > src/intel/compiler/brw_eu_validate.c| 64 -
> > src/intel/compiler/test_eu_validate.cpp | 122
> >
> > 2 files changed, 185 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/intel/compiler/brw_eu_validate.c
> > b/src/intel/compiler/brw_eu_validate.c
> > index 000a05cb6ac..203641fecb9 100644
> > --- a/src/intel/compiler/brw_eu_validate.c
> > +++ b/src/intel/compiler/brw_eu_validate.c
> > @@ -531,7 +531,69 @@
> > general_restrictions_based_on_operand_types(const struct
> > gen_device_info *devinf
> > exec_type_size == 8 && dst_type_size == 4)
> >dst_type_size = 8;
> >
> > - if (exec_type_size > dst_type_size) {
> > + /* From the BDW+ PRM:
> > +*
> > +*"There is no direct conversion from HF to DF or DF to HF.
> > +* There is no direct conversion from HF to Q/UQ or Q/UQ to
> > HF."
> > +*/
> > + enum brw_reg_type src0_type = brw_inst_src0_type(devinfo,
> > inst);
> > + ERROR_IF(brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV &&
>
> Why is only the MOV instruction handled here and below? Aren't other
> instructions able to do implicit conversions? Probably means you
> need
> to deal with two sources rather than one.
This comes from the programming notes of the MOV instruction (Volume
2a, Command Reference - Instructions - MOV), so it is described
specifically for the MOV instruction. I should probably have made this
clear in the comment.
> > +((dst_type == BRW_REGISTER_TYPE_HF &&
> > type_sz(src0_type) == 8) ||
> > + (dst_type_size == 8 && src0_type ==
> > BRW_REGISTER_TYPE_HF)),
> > +"There are no direct conversion between 64-bit types
> > and HF");
> > +
> > + /* From the BDW+ PRM:
> > +*
> > +* "Conversion between Integer and HF (Half Float) must be
> > +*DWord-aligned and strided by a DWord on the destination."
> > +*
> > +* But this seems to be expanded on CHV and SKL+ by:
> > +*
> > +* "There is a relaxed alignment rule for word destinations.
> > When
> > +*the destination type is word (UW, W, HF), destination
> > data types
> > +*can be aligned to either the lowest word or the second
> > lowest
> > +*word of the execution channel. This means the destination
> > data
> > +*words can be either all in the even word locations or all
> > in the
> > +*odd word locations."
> > +*
> > +* We do not implement the second rule as is though, since
> > empirical testing
> > +* shows inconsistencies:
> > +* - It suggests that packed 16-bit is not allowed, which is
> > not true.
> > +* - It suggests that conversions from Q/DF to W (which need
> > to be 64-bit
> > +* aligned on the destination) are not possible, which is
> > not true.
> > +* - It suggests that conversions from 16-bit executions
> > types to W need
> > +* to be 32-bit aligned, which doesn't seem to be
> > necessary.
> > +*
> > +* So from this rule we only validate the implication that
> > conversion from
> > +* F to HF needs to be DWord aligned too (in BDW this is
> > limited to
> > +* conversions from integer types).
> > +*/
> > + bool is_half_float_conversion =
> > + brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV &&
> > + dst_type != src0_type &&
> > + (dst_type == BRW_REGISTER_TYPE_HF || src0_type ==
> > BRW_REGISTER_TYPE_HF);
> > +
> > + if (is_half_float_conversion) {
> > + assert(devinfo->gen >= 8);
> > +
> > + if ((dst_type == BRW_REGISTER_TYPE_HF &&
> > brw_reg_type_is_integer(src0_type)) ||
> > + (brw_reg_type_is_integer(dst_type) && src0_type ==
> > BRW_REGISTER_TYPE_HF)) {
> > + ERROR_IF(dst_stride * dst_type_size != 4,
> > + "Conversions between integer and half-float must
> > be strided "
> > + "by a DWord on the destination");
> > +
> > + unsigned subreg = brw_inst_dst_da1_subreg_nr(devinfo,
> > inst);
> > + ERROR_IF(subreg % 4 != 0,
> > + "Conversions between integer and half-float must
> > be aligned "
> > + "to a DWord on the destination");
> > + } else if ((devinfo->is_cherryview || devinfo->gen >= 9) &&
> > + dst_type == BRW_REGISTER_TYPE_HF) {
> > + ERROR_IF(dst_stride != 2,
> > + "Conversions to HF must have either all words in
> > even word "
> > + "locations or all words in odd word locations");
> > + }
> > +
> > + } else if (exec_type_size > dst_type_size) {
> >if (!(dst_type_is_byte && inst_is_raw_move(devinfo, inst)))
> > {
> > ERROR_IF(dst_stride * dst_type_size != exec_type_size,
> >"Destination stride must be equal to the ratio
> > of the sizes "
> > diff --git a/src/intel/compiler/test_eu_validate.cpp
> > b/
Re: [Mesa-dev] [PATCH v4 34/40] intel/compiler: validate region restrictions for half-float conversions
Iago Toral Quiroga writes:
> ---
> src/intel/compiler/brw_eu_validate.c| 64 -
> src/intel/compiler/test_eu_validate.cpp | 122
> 2 files changed, 185 insertions(+), 1 deletion(-)
>
> diff --git a/src/intel/compiler/brw_eu_validate.c
> b/src/intel/compiler/brw_eu_validate.c
> index 000a05cb6ac..203641fecb9 100644
> --- a/src/intel/compiler/brw_eu_validate.c
> +++ b/src/intel/compiler/brw_eu_validate.c
> @@ -531,7 +531,69 @@ general_restrictions_based_on_operand_types(const struct
> gen_device_info *devinf
> exec_type_size == 8 && dst_type_size == 4)
>dst_type_size = 8;
>
> - if (exec_type_size > dst_type_size) {
> + /* From the BDW+ PRM:
> +*
> +*"There is no direct conversion from HF to DF or DF to HF.
> +* There is no direct conversion from HF to Q/UQ or Q/UQ to HF."
> +*/
> + enum brw_reg_type src0_type = brw_inst_src0_type(devinfo, inst);
> + ERROR_IF(brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV &&
Why is only the MOV instruction handled here and below? Aren't other
instructions able to do implicit conversions? Probably means you need
to deal with two sources rather than one.
> +((dst_type == BRW_REGISTER_TYPE_HF && type_sz(src0_type) == 8) ||
> + (dst_type_size == 8 && src0_type == BRW_REGISTER_TYPE_HF)),
> +"There are no direct conversion between 64-bit types and HF");
> +
> + /* From the BDW+ PRM:
> +*
> +* "Conversion between Integer and HF (Half Float) must be
> +*DWord-aligned and strided by a DWord on the destination."
> +*
> +* But this seems to be expanded on CHV and SKL+ by:
> +*
> +* "There is a relaxed alignment rule for word destinations. When
> +*the destination type is word (UW, W, HF), destination data types
> +*can be aligned to either the lowest word or the second lowest
> +*word of the execution channel. This means the destination data
> +*words can be either all in the even word locations or all in the
> +*odd word locations."
> +*
> +* We do not implement the second rule as is though, since empirical
> testing
> +* shows inconsistencies:
> +* - It suggests that packed 16-bit is not allowed, which is not true.
> +* - It suggests that conversions from Q/DF to W (which need to be
> 64-bit
> +* aligned on the destination) are not possible, which is not true.
> +* - It suggests that conversions from 16-bit executions types to W need
> +* to be 32-bit aligned, which doesn't seem to be necessary.
> +*
> +* So from this rule we only validate the implication that conversion from
> +* F to HF needs to be DWord aligned too (in BDW this is limited to
> +* conversions from integer types).
> +*/
> + bool is_half_float_conversion =
> + brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV &&
> + dst_type != src0_type &&
> + (dst_type == BRW_REGISTER_TYPE_HF || src0_type ==
> BRW_REGISTER_TYPE_HF);
> +
> + if (is_half_float_conversion) {
> + assert(devinfo->gen >= 8);
> +
> + if ((dst_type == BRW_REGISTER_TYPE_HF &&
> brw_reg_type_is_integer(src0_type)) ||
> + (brw_reg_type_is_integer(dst_type) && src0_type ==
> BRW_REGISTER_TYPE_HF)) {
> + ERROR_IF(dst_stride * dst_type_size != 4,
> + "Conversions between integer and half-float must be
> strided "
> + "by a DWord on the destination");
> +
> + unsigned subreg = brw_inst_dst_da1_subreg_nr(devinfo, inst);
> + ERROR_IF(subreg % 4 != 0,
> + "Conversions between integer and half-float must be
> aligned "
> + "to a DWord on the destination");
> + } else if ((devinfo->is_cherryview || devinfo->gen >= 9) &&
> + dst_type == BRW_REGISTER_TYPE_HF) {
> + ERROR_IF(dst_stride != 2,
> + "Conversions to HF must have either all words in even word
> "
> + "locations or all words in odd word locations");
> + }
> +
> + } else if (exec_type_size > dst_type_size) {
>if (!(dst_type_is_byte && inst_is_raw_move(devinfo, inst))) {
> ERROR_IF(dst_stride * dst_type_size != exec_type_size,
>"Destination stride must be equal to the ratio of the
> sizes "
> diff --git a/src/intel/compiler/test_eu_validate.cpp
> b/src/intel/compiler/test_eu_validate.cpp
> index 73300b23122..1557b6d2452 100644
> --- a/src/intel/compiler/test_eu_validate.cpp
> +++ b/src/intel/compiler/test_eu_validate.cpp
> @@ -848,6 +848,128 @@ TEST_P(validation_test,
> byte_destination_relaxed_alignment)
> }
> }
>
> +TEST_P(validation_test, half_float_conversion)
> +{
> + static const struct {
> + enum brw_reg_type dst_type;
> + enum brw_reg_type src_type;
> + unsigned dst_stride;
> + unsigned dst_subnr;
> + bool expected_result;
> + } inst[] = {
> +#defi
Re: [Mesa-dev] [PATCH v4 34/40] intel/compiler: validate region restrictions for half-float conversions
On Sat, 2019-02-16 at 09:40 -0600, Jason Ekstrand wrote:
> On Tue, Feb 12, 2019 at 11:53 AM Iago Toral Quiroga <
> ito...@igalia.com> wrote:
> > ---
> >
> > src/intel/compiler/brw_eu_validate.c| 64 -
> >
> > src/intel/compiler/test_eu_validate.cpp | 122
> >
> >
> > 2 files changed, 185 insertions(+), 1 deletion(-)
> >
> >
> >
> > diff --git a/src/intel/compiler/brw_eu_validate.c
> > b/src/intel/compiler/brw_eu_validate.c
> >
> > index 000a05cb6ac..203641fecb9 100644
> >
> > --- a/src/intel/compiler/brw_eu_validate.c
> >
> > +++ b/src/intel/compiler/brw_eu_validate.c
> >
> > @@ -531,7 +531,69 @@
> > general_restrictions_based_on_operand_types(const struct
> > gen_device_info *devinf
> >
> > exec_type_size == 8 && dst_type_size == 4)
> >
> >dst_type_size = 8;
> >
> >
> >
> > - if (exec_type_size > dst_type_size) {
> >
> > + /* From the BDW+ PRM:
> >
> > +*
> >
> > +*"There is no direct conversion from HF to DF or DF to HF.
> >
> > +* There is no direct conversion from HF to Q/UQ or Q/UQ to
> > HF."
> >
> > +*/
> >
> > + enum brw_reg_type src0_type = brw_inst_src0_type(devinfo,
> > inst);
> >
> > + ERROR_IF(brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV &&
> >
> > +((dst_type == BRW_REGISTER_TYPE_HF &&
> > type_sz(src0_type) == 8) ||
> >
> > + (dst_type_size == 8 && src0_type ==
> > BRW_REGISTER_TYPE_HF)),
> >
> > +"There are no direct conversion between 64-bit types
> > and HF");
> >
> > +
> >
> > + /* From the BDW+ PRM:
> >
> > +*
> >
> > +* "Conversion between Integer and HF (Half Float) must be
> >
> > +*DWord-aligned and strided by a DWord on the destination."
> >
> > +*
> >
> > +* But this seems to be expanded on CHV and SKL+ by:
> >
> > +*
> >
> > +* "There is a relaxed alignment rule for word destinations.
> > When
> >
> > +*the destination type is word (UW, W, HF), destination
> > data types
> >
> > +*can be aligned to either the lowest word or the second
> > lowest
> >
> > +*word of the execution channel. This means the destination
> > data
> >
> > +*words can be either all in the even word locations or all
> > in the
> >
> > +*odd word locations."
> >
> > +*
> >
> > +* We do not implement the second rule as is though, since
> > empirical testing
> >
> > +* shows inconsistencies:
> >
> > +* - It suggests that packed 16-bit is not allowed, which is
> > not true.
> >
> > +* - It suggests that conversions from Q/DF to W (which need
> > to be 64-bit
> >
> > +* aligned on the destination) are not possible, which is
> > not true.
> >
> > +* - It suggests that conversions from 16-bit executions
> > types to W need
> >
> > +* to be 32-bit aligned, which doesn't seem to be
> > necessary.
> >
> > +*
> >
> > +* So from this rule we only validate the implication that
> > conversion from
> >
> > +* F to HF needs to be DWord aligned too (in BDW this is
> > limited to
> >
> > +* conversions from integer types).
> >
> > +*/
> >
> > + bool is_half_float_conversion =
> >
> > + brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV &&
> >
> > + dst_type != src0_type &&
> >
> > + (dst_type == BRW_REGISTER_TYPE_HF || src0_type ==
> > BRW_REGISTER_TYPE_HF);
> >
> > +
> >
> > + if (is_half_float_conversion) {
> >
> > + assert(devinfo->gen >= 8);
> >
> > +
> >
> > + if ((dst_type == BRW_REGISTER_TYPE_HF &&
> > brw_reg_type_is_integer(src0_type)) ||
> >
> > + (brw_reg_type_is_integer(dst_type) && src0_type ==
> > BRW_REGISTER_TYPE_HF)) {
> >
> > + ERROR_IF(dst_stride * dst_type_size != 4,
> >
> > + "Conversions between integer and half-float must
> > be strided "
> >
> > + "by a DWord on the destination");
>
> Does this mean stride must be 4B or does it mean a multiple of 4B?
>
> > +
> >
> > + unsigned subreg = brw_inst_dst_da1_subreg_nr(devinfo,
> > inst);
> >
> > + ERROR_IF(subreg % 4 != 0,
> >
> > + "Conversions between integer and half-float must
> > be aligned "
> >
> > + "to a DWord on the destination");
> >
> > + } else if ((devinfo->is_cherryview || devinfo->gen >= 9) &&
> >
> > + dst_type == BRW_REGISTER_TYPE_HF) {
> >
> > + ERROR_IF(dst_stride != 2,
>
> Should this be dst_stride != 2 or dst_stride == 1? If dst_stride
> were, say 4, that would place them all in even or all in odd
> locations. It's only if dst_stride == 1 that you end up with both
> even and odd.
I think this needs to be exactly a DWord for both the stride and the
alignment. When Curro explained this he made the case that what is
probably happening under the hood is that there is a promotion of the
exec type to 32-bit, and then the following general
Re: [Mesa-dev] [PATCH v4 34/40] intel/compiler: validate region restrictions for half-float conversions
On Tue, Feb 12, 2019 at 11:53 AM Iago Toral Quiroga
wrote:
> ---
> src/intel/compiler/brw_eu_validate.c| 64 -
> src/intel/compiler/test_eu_validate.cpp | 122
> 2 files changed, 185 insertions(+), 1 deletion(-)
>
> diff --git a/src/intel/compiler/brw_eu_validate.c
> b/src/intel/compiler/brw_eu_validate.c
> index 000a05cb6ac..203641fecb9 100644
> --- a/src/intel/compiler/brw_eu_validate.c
> +++ b/src/intel/compiler/brw_eu_validate.c
> @@ -531,7 +531,69 @@ general_restrictions_based_on_operand_types(const
> struct gen_device_info *devinf
> exec_type_size == 8 && dst_type_size == 4)
>dst_type_size = 8;
>
> - if (exec_type_size > dst_type_size) {
> + /* From the BDW+ PRM:
> +*
> +*"There is no direct conversion from HF to DF or DF to HF.
> +* There is no direct conversion from HF to Q/UQ or Q/UQ to HF."
> +*/
> + enum brw_reg_type src0_type = brw_inst_src0_type(devinfo, inst);
> + ERROR_IF(brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV &&
> +((dst_type == BRW_REGISTER_TYPE_HF && type_sz(src0_type) ==
> 8) ||
> + (dst_type_size == 8 && src0_type == BRW_REGISTER_TYPE_HF)),
> +"There are no direct conversion between 64-bit types and HF");
> +
> + /* From the BDW+ PRM:
> +*
> +* "Conversion between Integer and HF (Half Float) must be
> +*DWord-aligned and strided by a DWord on the destination."
> +*
> +* But this seems to be expanded on CHV and SKL+ by:
> +*
> +* "There is a relaxed alignment rule for word destinations. When
> +*the destination type is word (UW, W, HF), destination data types
> +*can be aligned to either the lowest word or the second lowest
> +*word of the execution channel. This means the destination data
> +*words can be either all in the even word locations or all in the
> +*odd word locations."
> +*
> +* We do not implement the second rule as is though, since empirical
> testing
> +* shows inconsistencies:
> +* - It suggests that packed 16-bit is not allowed, which is not
> true.
> +* - It suggests that conversions from Q/DF to W (which need to be
> 64-bit
> +* aligned on the destination) are not possible, which is not true.
> +* - It suggests that conversions from 16-bit executions types to W
> need
> +* to be 32-bit aligned, which doesn't seem to be necessary.
> +*
> +* So from this rule we only validate the implication that conversion
> from
> +* F to HF needs to be DWord aligned too (in BDW this is limited to
> +* conversions from integer types).
> +*/
> + bool is_half_float_conversion =
> + brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV &&
> + dst_type != src0_type &&
> + (dst_type == BRW_REGISTER_TYPE_HF || src0_type ==
> BRW_REGISTER_TYPE_HF);
> +
> + if (is_half_float_conversion) {
> + assert(devinfo->gen >= 8);
> +
> + if ((dst_type == BRW_REGISTER_TYPE_HF &&
> brw_reg_type_is_integer(src0_type)) ||
> + (brw_reg_type_is_integer(dst_type) && src0_type ==
> BRW_REGISTER_TYPE_HF)) {
> + ERROR_IF(dst_stride * dst_type_size != 4,
> + "Conversions between integer and half-float must be
> strided "
> + "by a DWord on the destination");
>
Does this mean stride must be 4B or does it mean a multiple of 4B?
> +
> + unsigned subreg = brw_inst_dst_da1_subreg_nr(devinfo, inst);
> + ERROR_IF(subreg % 4 != 0,
> + "Conversions between integer and half-float must be
> aligned "
> + "to a DWord on the destination");
> + } else if ((devinfo->is_cherryview || devinfo->gen >= 9) &&
> + dst_type == BRW_REGISTER_TYPE_HF) {
> + ERROR_IF(dst_stride != 2,
>
Should this be dst_stride != 2 or dst_stride == 1? If dst_stride were, say
4, that would place them all in even or all in odd locations. It's only if
dst_stride == 1 that you end up with both even and odd.
> + "Conversions to HF must have either all words in even
> word "
> + "locations or all words in odd word locations");
> + }
> +
> + } else if (exec_type_size > dst_type_size) {
>if (!(dst_type_is_byte && inst_is_raw_move(devinfo, inst))) {
> ERROR_IF(dst_stride * dst_type_size != exec_type_size,
>"Destination stride must be equal to the ratio of the
> sizes "
> diff --git a/src/intel/compiler/test_eu_validate.cpp
> b/src/intel/compiler/test_eu_validate.cpp
> index 73300b23122..1557b6d2452 100644
> --- a/src/intel/compiler/test_eu_validate.cpp
> +++ b/src/intel/compiler/test_eu_validate.cpp
> @@ -848,6 +848,128 @@ TEST_P(validation_test,
> byte_destination_relaxed_alignment)
> }
> }
>
> +TEST_P(validation_test, half_float_conversion)
> +{
> + static const struct {
> + enum brw_reg_type dst_type;
> + enum brw_reg_type src_
[Mesa-dev] [PATCH v4 34/40] intel/compiler: validate region restrictions for half-float conversions
---
src/intel/compiler/brw_eu_validate.c| 64 -
src/intel/compiler/test_eu_validate.cpp | 122
2 files changed, 185 insertions(+), 1 deletion(-)
diff --git a/src/intel/compiler/brw_eu_validate.c
b/src/intel/compiler/brw_eu_validate.c
index 000a05cb6ac..203641fecb9 100644
--- a/src/intel/compiler/brw_eu_validate.c
+++ b/src/intel/compiler/brw_eu_validate.c
@@ -531,7 +531,69 @@ general_restrictions_based_on_operand_types(const struct
gen_device_info *devinf
exec_type_size == 8 && dst_type_size == 4)
dst_type_size = 8;
- if (exec_type_size > dst_type_size) {
+ /* From the BDW+ PRM:
+*
+*"There is no direct conversion from HF to DF or DF to HF.
+* There is no direct conversion from HF to Q/UQ or Q/UQ to HF."
+*/
+ enum brw_reg_type src0_type = brw_inst_src0_type(devinfo, inst);
+ ERROR_IF(brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV &&
+((dst_type == BRW_REGISTER_TYPE_HF && type_sz(src0_type) == 8) ||
+ (dst_type_size == 8 && src0_type == BRW_REGISTER_TYPE_HF)),
+"There are no direct conversion between 64-bit types and HF");
+
+ /* From the BDW+ PRM:
+*
+* "Conversion between Integer and HF (Half Float) must be
+*DWord-aligned and strided by a DWord on the destination."
+*
+* But this seems to be expanded on CHV and SKL+ by:
+*
+* "There is a relaxed alignment rule for word destinations. When
+*the destination type is word (UW, W, HF), destination data types
+*can be aligned to either the lowest word or the second lowest
+*word of the execution channel. This means the destination data
+*words can be either all in the even word locations or all in the
+*odd word locations."
+*
+* We do not implement the second rule as is though, since empirical testing
+* shows inconsistencies:
+* - It suggests that packed 16-bit is not allowed, which is not true.
+* - It suggests that conversions from Q/DF to W (which need to be 64-bit
+* aligned on the destination) are not possible, which is not true.
+* - It suggests that conversions from 16-bit executions types to W need
+* to be 32-bit aligned, which doesn't seem to be necessary.
+*
+* So from this rule we only validate the implication that conversion from
+* F to HF needs to be DWord aligned too (in BDW this is limited to
+* conversions from integer types).
+*/
+ bool is_half_float_conversion =
+ brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV &&
+ dst_type != src0_type &&
+ (dst_type == BRW_REGISTER_TYPE_HF || src0_type == BRW_REGISTER_TYPE_HF);
+
+ if (is_half_float_conversion) {
+ assert(devinfo->gen >= 8);
+
+ if ((dst_type == BRW_REGISTER_TYPE_HF &&
brw_reg_type_is_integer(src0_type)) ||
+ (brw_reg_type_is_integer(dst_type) && src0_type ==
BRW_REGISTER_TYPE_HF)) {
+ ERROR_IF(dst_stride * dst_type_size != 4,
+ "Conversions between integer and half-float must be strided "
+ "by a DWord on the destination");
+
+ unsigned subreg = brw_inst_dst_da1_subreg_nr(devinfo, inst);
+ ERROR_IF(subreg % 4 != 0,
+ "Conversions between integer and half-float must be aligned "
+ "to a DWord on the destination");
+ } else if ((devinfo->is_cherryview || devinfo->gen >= 9) &&
+ dst_type == BRW_REGISTER_TYPE_HF) {
+ ERROR_IF(dst_stride != 2,
+ "Conversions to HF must have either all words in even word "
+ "locations or all words in odd word locations");
+ }
+
+ } else if (exec_type_size > dst_type_size) {
if (!(dst_type_is_byte && inst_is_raw_move(devinfo, inst))) {
ERROR_IF(dst_stride * dst_type_size != exec_type_size,
"Destination stride must be equal to the ratio of the sizes "
diff --git a/src/intel/compiler/test_eu_validate.cpp
b/src/intel/compiler/test_eu_validate.cpp
index 73300b23122..1557b6d2452 100644
--- a/src/intel/compiler/test_eu_validate.cpp
+++ b/src/intel/compiler/test_eu_validate.cpp
@@ -848,6 +848,128 @@ TEST_P(validation_test,
byte_destination_relaxed_alignment)
}
}
+TEST_P(validation_test, half_float_conversion)
+{
+ static const struct {
+ enum brw_reg_type dst_type;
+ enum brw_reg_type src_type;
+ unsigned dst_stride;
+ unsigned dst_subnr;
+ bool expected_result;
+ } inst[] = {
+#define INST(dst_type, src_type, dst_stride, dst_subnr, expected_result) \
+ { \
+ BRW_REGISTER_TYPE_##dst_type,\
+ BRW_REGISTER_TYPE_##src_type,\
+ BRW_HORIZONTAL_STRIDE_##dst_stride, \
+ dst_subnr,
