Re: [Mesa-dev] [PATCH] intel/compiler: update validator to account for half-float exec type promotion
Iago Toral writes: > On Mon, 2019-02-04 at 08:50 +0100, Iago Toral wrote: >> On Fri, 2019-02-01 at 11:23 -0800, Francisco Jerez wrote: >> > Iago Toral writes: >> > >> > > On Fri, 2019-01-25 at 12:54 -0800, Francisco Jerez wrote: >> > > > Iago Toral writes: >> > > > >> > > > > On Thu, 2019-01-24 at 11:45 -0800, Francisco Jerez wrote: >> > > > > > Iago Toral writes: >> > > > > > >> > > > > > > On Wed, 2019-01-23 at 06:03 -0800, Francisco Jerez wrote: >> > > > > > > > Iago Toral Quiroga writes: >> > > > > > > > >> > > > > > > > > Commit c84ec70b3a72 implemented execution type >> > > > > > > > > promotion to >> > > > > > > > > 32- >> > > > > > > > > bit >> > > > > > > > > for >> > > > > > > > > conversions involving half-float registers, which >> > > > > > > > > empirical >> > > > > > > > > testing >> > > > > > > > > suggested >> > > > > > > > > was required, but it did not incorporate this change >> > > > > > > > > into >> > > > > > > > > the >> > > > > > > > > assembly validator >> > > > > > > > > logic. This commits adds that, preventing validation >> > > > > > > > > errors >> > > > > > > > > like >> > > > > > > > > this: >> > > > > > > > > >> > > > > > > > >> > > > > > > > I don't think we should be validating empirical >> > > > > > > > assumptions >> > > > > > > > in >> > > > > > > > the EU >> > > > > > > > validator. >> > > > > > > >> > > > > > > I am not sure I get your point, isn't c84ec70b3a72 also >> > > > > > > based >> > > > > > > on >> > > > > > > empirical testing after all? >> > > > > > > >> > > > > > >> > > > > > To some extent, but it doesn't attempt to enforce ISA >> > > > > > restrictions >> > > > > > based >> > > > > > on information obtained empirically. >> > > > > > >> > > > > > > >> > > > > > > > > mov(16) g9<4>B g3<16,8,2>HF { align1 1H }; >> > > > > > > > > ERROR: Destination stride must be equal to the ratio >> > > > > > > > > of >> > > > > > > > > the >> > > > > > > > > sizes >> > > > > > > > > of the >> > > > > > > > >execution data type to the destination type >> > > > > > > > > >> > > > > > > > > Fixes: c84ec70b3a72 "intel/fs: Promote execution type >> > > > > > > > > to >> > > > > > > > > 32-bit >> > > > > > > > > when any half-float conversion is needed." >> > > > > > > > >> > > > > > > > I don't think this "fixes" anything that ever worked. >> > > > > > > >> > > > > > > It is true that the code in that trace above is not >> > > > > > > something >> > > > > > > we >> > > > > > > can >> > > > > > > produce right now, because it is a conversion from HF to >> > > > > > > B >> > > > > > > and >> > > > > > > that >> > > > > > > should only happen within the context of >> > > > > > > VK_KHR_shader_float16_int8, >> > > > > > > however, this is a consequence of the fact that since >> > > > > > > c84ec70b3a72 >> > > > > > > there is an inconsistency between what we do at the IR >> > > > > > > level >> > > > > > > regarding >> > > > > > > execution size of HF conversions and what the EU >> > > > > > > validator >> > > > > > > is >> > > > > > > doing, >> > > > > > > and from that perspective this is really fixing an >> > > > > > > inconsistency >> > > > > > > that >> > > > > > > didn't exist before, and I thought we would want to >> > > > > > > address >> > > > > > > that >> > > > > > > sooner >> > > > > > > rather than later and track it down to the original >> > > > > > > change >> > > > > > > that >> > > > > > > introduced that inconsistency so we know where this is >> > > > > > > coming >> > > > > > > from. >> > > > > > > >> > > > > > >> > > > > > The "inconsistency" between the IR's get_exec_type() and >> > > > > > the >> > > > > > EU >> > > > > > validator's execution_type() has existed ever since >> > > > > > a05b6f25bf4bfad7 >> > > > > > removed the HF assert from get_exec_type() without actually >> > > > > > implementing >> > > > > > the code required to handle HF operands (which is what my >> > > > > > commit >> > > > > > c84ec70b3a72 did). >> > > > > >> > > > > I agree with the fact that since a05b6f25bf4bfad7 the >> > > > > validator >> > > > > could >> > > > > reject valid code and that had nothing to do with your patch, >> > > > >> > > > The validator rejected the same valid HF code since it was >> > > > written, >> > > > that >> > > > had nothing to do with neither a05b6f25bf4bfad7 nor with my >> > > > patch, >> > > > and >> > > > it is the real problem this patch was working around. >> > > > >> > > > > but the inconsistency I am talking about here, that this >> > > > > patch >> > > > > fixes, >> > > > > is the one about get_exec_type() in the IR and >> > > > > execution_type() >> > > > > in >> > > > > the >> > > > > validator doing different things for HF instructions, which >> > > > > only >> > > > > exists since your patch and which you discuss below. >> > > > > >> > > > >> > > > The "inconsistency" exists ever since get_exec_type() was >> > > > introduced >> > > > without correct handling of HF types (even though >> > > >
Re: [Mesa-dev] [PATCH] intel/compiler: update validator to account for half-float exec type promotion
On Mon, 2019-02-04 at 08:50 +0100, Iago Toral wrote: > On Fri, 2019-02-01 at 11:23 -0800, Francisco Jerez wrote: > > Iago Toral writes: > > > > > On Fri, 2019-01-25 at 12:54 -0800, Francisco Jerez wrote: > > > > Iago Toral writes: > > > > > > > > > On Thu, 2019-01-24 at 11:45 -0800, Francisco Jerez wrote: > > > > > > Iago Toral writes: > > > > > > > > > > > > > On Wed, 2019-01-23 at 06:03 -0800, Francisco Jerez wrote: > > > > > > > > Iago Toral Quiroga writes: > > > > > > > > > > > > > > > > > Commit c84ec70b3a72 implemented execution type > > > > > > > > > promotion to > > > > > > > > > 32- > > > > > > > > > bit > > > > > > > > > for > > > > > > > > > conversions involving half-float registers, which > > > > > > > > > empirical > > > > > > > > > testing > > > > > > > > > suggested > > > > > > > > > was required, but it did not incorporate this change > > > > > > > > > into > > > > > > > > > the > > > > > > > > > assembly validator > > > > > > > > > logic. This commits adds that, preventing validation > > > > > > > > > errors > > > > > > > > > like > > > > > > > > > this: > > > > > > > > > > > > > > > > > > > > > > > > > I don't think we should be validating empirical > > > > > > > > assumptions > > > > > > > > in > > > > > > > > the EU > > > > > > > > validator. > > > > > > > > > > > > > > I am not sure I get your point, isn't c84ec70b3a72 also > > > > > > > based > > > > > > > on > > > > > > > empirical testing after all? > > > > > > > > > > > > > > > > > > > To some extent, but it doesn't attempt to enforce ISA > > > > > > restrictions > > > > > > based > > > > > > on information obtained empirically. > > > > > > > > > > > > > > > > > > > > > > mov(16) g9<4>B g3<16,8,2>HF { align1 1H }; > > > > > > > > > ERROR: Destination stride must be equal to the ratio > > > > > > > > > of > > > > > > > > > the > > > > > > > > > sizes > > > > > > > > > of the > > > > > > > > >execution data type to the destination type > > > > > > > > > > > > > > > > > > Fixes: c84ec70b3a72 "intel/fs: Promote execution type > > > > > > > > > to > > > > > > > > > 32-bit > > > > > > > > > when any half-float conversion is needed." > > > > > > > > > > > > > > > > I don't think this "fixes" anything that ever worked. > > > > > > > > > > > > > > It is true that the code in that trace above is not > > > > > > > something > > > > > > > we > > > > > > > can > > > > > > > produce right now, because it is a conversion from HF to > > > > > > > B > > > > > > > and > > > > > > > that > > > > > > > should only happen within the context of > > > > > > > VK_KHR_shader_float16_int8, > > > > > > > however, this is a consequence of the fact that since > > > > > > > c84ec70b3a72 > > > > > > > there is an inconsistency between what we do at the IR > > > > > > > level > > > > > > > regarding > > > > > > > execution size of HF conversions and what the EU > > > > > > > validator > > > > > > > is > > > > > > > doing, > > > > > > > and from that perspective this is really fixing an > > > > > > > inconsistency > > > > > > > that > > > > > > > didn't exist before, and I thought we would want to > > > > > > > address > > > > > > > that > > > > > > > sooner > > > > > > > rather than later and track it down to the original > > > > > > > change > > > > > > > that > > > > > > > introduced that inconsistency so we know where this is > > > > > > > coming > > > > > > > from. > > > > > > > > > > > > > > > > > > > The "inconsistency" between the IR's get_exec_type() and > > > > > > the > > > > > > EU > > > > > > validator's execution_type() has existed ever since > > > > > > a05b6f25bf4bfad7 > > > > > > removed the HF assert from get_exec_type() without actually > > > > > > implementing > > > > > > the code required to handle HF operands (which is what my > > > > > > commit > > > > > > c84ec70b3a72 did). > > > > > > > > > > I agree with the fact that since a05b6f25bf4bfad7 the > > > > > validator > > > > > could > > > > > reject valid code and that had nothing to do with your patch, > > > > > > > > The validator rejected the same valid HF code since it was > > > > written, > > > > that > > > > had nothing to do with neither a05b6f25bf4bfad7 nor with my > > > > patch, > > > > and > > > > it is the real problem this patch was working around. > > > > > > > > > but the inconsistency I am talking about here, that this > > > > > patch > > > > > fixes, > > > > > is the one about get_exec_type() in the IR and > > > > > execution_type() > > > > > in > > > > > the > > > > > validator doing different things for HF instructions, which > > > > > only > > > > > exists since your patch and which you discuss below. > > > > > > > > > > > > > The "inconsistency" exists ever since get_exec_type() was > > > > introduced > > > > without correct handling of HF types (even though > > > > execution_type() > > > > already attempted to handle it). And I disagree that it's a > > > > real > > > > inconsistency except due to the fact that the validator is >
Re: [Mesa-dev] [PATCH] intel/compiler: update validator to account for half-float exec type promotion
On Fri, 2019-02-01 at 11:23 -0800, Francisco Jerez wrote: > Iago Toral writes: > > > On Fri, 2019-01-25 at 12:54 -0800, Francisco Jerez wrote: > > > Iago Toral writes: > > > > > > > On Thu, 2019-01-24 at 11:45 -0800, Francisco Jerez wrote: > > > > > Iago Toral writes: > > > > > > > > > > > On Wed, 2019-01-23 at 06:03 -0800, Francisco Jerez wrote: > > > > > > > Iago Toral Quiroga writes: > > > > > > > > > > > > > > > Commit c84ec70b3a72 implemented execution type > > > > > > > > promotion to > > > > > > > > 32- > > > > > > > > bit > > > > > > > > for > > > > > > > > conversions involving half-float registers, which > > > > > > > > empirical > > > > > > > > testing > > > > > > > > suggested > > > > > > > > was required, but it did not incorporate this change > > > > > > > > into > > > > > > > > the > > > > > > > > assembly validator > > > > > > > > logic. This commits adds that, preventing validation > > > > > > > > errors > > > > > > > > like > > > > > > > > this: > > > > > > > > > > > > > > > > > > > > > > I don't think we should be validating empirical > > > > > > > assumptions > > > > > > > in > > > > > > > the EU > > > > > > > validator. > > > > > > > > > > > > I am not sure I get your point, isn't c84ec70b3a72 also > > > > > > based > > > > > > on > > > > > > empirical testing after all? > > > > > > > > > > > > > > > > To some extent, but it doesn't attempt to enforce ISA > > > > > restrictions > > > > > based > > > > > on information obtained empirically. > > > > > > > > > > > > > > > > > > > mov(16) g9<4>B g3<16,8,2>HF { align1 1H }; > > > > > > > > ERROR: Destination stride must be equal to the ratio of > > > > > > > > the > > > > > > > > sizes > > > > > > > > of the > > > > > > > >execution data type to the destination type > > > > > > > > > > > > > > > > Fixes: c84ec70b3a72 "intel/fs: Promote execution type > > > > > > > > to > > > > > > > > 32-bit > > > > > > > > when any half-float conversion is needed." > > > > > > > > > > > > > > I don't think this "fixes" anything that ever worked. > > > > > > > > > > > > It is true that the code in that trace above is not > > > > > > something > > > > > > we > > > > > > can > > > > > > produce right now, because it is a conversion from HF to B > > > > > > and > > > > > > that > > > > > > should only happen within the context of > > > > > > VK_KHR_shader_float16_int8, > > > > > > however, this is a consequence of the fact that since > > > > > > c84ec70b3a72 > > > > > > there is an inconsistency between what we do at the IR > > > > > > level > > > > > > regarding > > > > > > execution size of HF conversions and what the EU validator > > > > > > is > > > > > > doing, > > > > > > and from that perspective this is really fixing an > > > > > > inconsistency > > > > > > that > > > > > > didn't exist before, and I thought we would want to address > > > > > > that > > > > > > sooner > > > > > > rather than later and track it down to the original change > > > > > > that > > > > > > introduced that inconsistency so we know where this is > > > > > > coming > > > > > > from. > > > > > > > > > > > > > > > > The "inconsistency" between the IR's get_exec_type() and the > > > > > EU > > > > > validator's execution_type() has existed ever since > > > > > a05b6f25bf4bfad7 > > > > > removed the HF assert from get_exec_type() without actually > > > > > implementing > > > > > the code required to handle HF operands (which is what my > > > > > commit > > > > > c84ec70b3a72 did). > > > > > > > > I agree with the fact that since a05b6f25bf4bfad7 the validator > > > > could > > > > reject valid code and that had nothing to do with your patch, > > > > > > The validator rejected the same valid HF code since it was > > > written, > > > that > > > had nothing to do with neither a05b6f25bf4bfad7 nor with my > > > patch, > > > and > > > it is the real problem this patch was working around. > > > > > > > but the inconsistency I am talking about here, that this patch > > > > fixes, > > > > is the one about get_exec_type() in the IR and execution_type() > > > > in > > > > the > > > > validator doing different things for HF instructions, which > > > > only > > > > exists since your patch and which you discuss below. > > > > > > > > > > The "inconsistency" exists ever since get_exec_type() was > > > introduced > > > without correct handling of HF types (even though > > > execution_type() > > > already attempted to handle it). And I disagree that it's a real > > > inconsistency except due to the fact that the validator is > > > incorrectly > > > attempting to validate the alignment of the destination region > > > according > > > to a rule that doesn't apply to HF types. > > > > > > > > > Anyway, that was my rationale for the Fixes tag, but if you > > > > > > think > > > > > > this > > > > > > is not useful I am happy to drop this patch and just > > > > > > include it > > > > > > as > > > > > > part > > > > > > of my series without the tag. > > > > > >
Re: [Mesa-dev] [PATCH] intel/compiler: update validator to account for half-float exec type promotion
Iago Toral writes: > On Fri, 2019-01-25 at 12:54 -0800, Francisco Jerez wrote: >> Iago Toral writes: >> >> > On Thu, 2019-01-24 at 11:45 -0800, Francisco Jerez wrote: >> > > Iago Toral writes: >> > > >> > > > On Wed, 2019-01-23 at 06:03 -0800, Francisco Jerez wrote: >> > > > > Iago Toral Quiroga writes: >> > > > > >> > > > > > Commit c84ec70b3a72 implemented execution type promotion to >> > > > > > 32- >> > > > > > bit >> > > > > > for >> > > > > > conversions involving half-float registers, which empirical >> > > > > > testing >> > > > > > suggested >> > > > > > was required, but it did not incorporate this change into >> > > > > > the >> > > > > > assembly validator >> > > > > > logic. This commits adds that, preventing validation errors >> > > > > > like >> > > > > > this: >> > > > > > >> > > > > >> > > > > I don't think we should be validating empirical assumptions >> > > > > in >> > > > > the EU >> > > > > validator. >> > > > >> > > > I am not sure I get your point, isn't c84ec70b3a72 also based >> > > > on >> > > > empirical testing after all? >> > > > >> > > >> > > To some extent, but it doesn't attempt to enforce ISA >> > > restrictions >> > > based >> > > on information obtained empirically. >> > > >> > > > >> > > > > > mov(16) g9<4>B g3<16,8,2>HF { align1 1H }; >> > > > > > ERROR: Destination stride must be equal to the ratio of the >> > > > > > sizes >> > > > > > of the >> > > > > >execution data type to the destination type >> > > > > > >> > > > > > Fixes: c84ec70b3a72 "intel/fs: Promote execution type to >> > > > > > 32-bit >> > > > > > when any half-float conversion is needed." >> > > > > >> > > > > I don't think this "fixes" anything that ever worked. >> > > > >> > > > It is true that the code in that trace above is not something >> > > > we >> > > > can >> > > > produce right now, because it is a conversion from HF to B and >> > > > that >> > > > should only happen within the context of >> > > > VK_KHR_shader_float16_int8, >> > > > however, this is a consequence of the fact that since >> > > > c84ec70b3a72 >> > > > there is an inconsistency between what we do at the IR level >> > > > regarding >> > > > execution size of HF conversions and what the EU validator is >> > > > doing, >> > > > and from that perspective this is really fixing an >> > > > inconsistency >> > > > that >> > > > didn't exist before, and I thought we would want to address >> > > > that >> > > > sooner >> > > > rather than later and track it down to the original change that >> > > > introduced that inconsistency so we know where this is coming >> > > > from. >> > > > >> > > >> > > The "inconsistency" between the IR's get_exec_type() and the EU >> > > validator's execution_type() has existed ever since >> > > a05b6f25bf4bfad7 >> > > removed the HF assert from get_exec_type() without actually >> > > implementing >> > > the code required to handle HF operands (which is what my commit >> > > c84ec70b3a72 did). >> > >> > I agree with the fact that since a05b6f25bf4bfad7 the validator >> > could >> > reject valid code and that had nothing to do with your patch, >> >> The validator rejected the same valid HF code since it was written, >> that >> had nothing to do with neither a05b6f25bf4bfad7 nor with my patch, >> and >> it is the real problem this patch was working around. >> >> > but the inconsistency I am talking about here, that this patch >> > fixes, >> > is the one about get_exec_type() in the IR and execution_type() in >> > the >> > validator doing different things for HF instructions, which only >> > exists since your patch and which you discuss below. >> > >> >> The "inconsistency" exists ever since get_exec_type() was introduced >> without correct handling of HF types (even though execution_type() >> already attempted to handle it). And I disagree that it's a real >> inconsistency except due to the fact that the validator is >> incorrectly >> attempting to validate the alignment of the destination region >> according >> to a rule that doesn't apply to HF types. >> >> > > > Anyway, that was my rationale for the Fixes tag, but if you >> > > > think >> > > > this >> > > > is not useful I am happy to drop this patch and just include it >> > > > as >> > > > part >> > > > of my series without the tag. >> > > > >> > > >> > > I'd like to see the actual regioning restrictions for HF types >> > > implemented in the EU validator as part of your series. >> > >> > Ok, let's see if we can agree on what restrictions should we >> > implement >> > then. I can implement this restriction as documented: >> > >> > "Conversion between Integer and HF (Half Float) must be DWord- >> > aligned >> > and strided by a DWord on the destination" >> > >> > Instead of trying to apply the general one that is currently >> > breaking. >> > That will fix the assertion issue. I guess my issue with it was >> > that >> > going by your analysis this restriction is not telling the full >> > picture, this is what
Re: [Mesa-dev] [PATCH] intel/compiler: update validator to account for half-float exec type promotion
On Fri, 2019-02-01 at 12:34 +0100, Iago Toral wrote: > On Fri, 2019-01-25 at 12:54 -0800, Francisco Jerez wrote: > > Iago Toral writes: > > > > > On Thu, 2019-01-24 at 11:45 -0800, Francisco Jerez wrote: > > > > Iago Toral writes: > > > > > > > > > On Wed, 2019-01-23 at 06:03 -0800, Francisco Jerez wrote: > > > > > > Iago Toral Quiroga writes: > > > > > > > > > > > > > Commit c84ec70b3a72 implemented execution type promotion > > > > > > > to > > > > > > > 32- > > > > > > > bit > > > > > > > for > > > > > > > conversions involving half-float registers, which > > > > > > > empirical > > > > > > > testing > > > > > > > suggested > > > > > > > was required, but it did not incorporate this change into > > > > > > > the > > > > > > > assembly validator > > > > > > > logic. This commits adds that, preventing validation > > > > > > > errors > > > > > > > like > > > > > > > this: > > > > > > > > > > > > > > > > > > > I don't think we should be validating empirical assumptions > > > > > > in > > > > > > the EU > > > > > > validator. > > > > > > > > > > I am not sure I get your point, isn't c84ec70b3a72 also based > > > > > on > > > > > empirical testing after all? > > > > > > > > > > > > > To some extent, but it doesn't attempt to enforce ISA > > > > restrictions > > > > based > > > > on information obtained empirically. > > > > > > > > > > > > > > > > mov(16) g9<4>B g3<16,8,2>HF { align1 1H }; > > > > > > > ERROR: Destination stride must be equal to the ratio of > > > > > > > the > > > > > > > sizes > > > > > > > of the > > > > > > >execution data type to the destination type > > > > > > > > > > > > > > Fixes: c84ec70b3a72 "intel/fs: Promote execution type to > > > > > > > 32-bit > > > > > > > when any half-float conversion is needed." > > > > > > > > > > > > I don't think this "fixes" anything that ever worked. > > > > > > > > > > It is true that the code in that trace above is not something > > > > > we > > > > > can > > > > > produce right now, because it is a conversion from HF to B > > > > > and > > > > > that > > > > > should only happen within the context of > > > > > VK_KHR_shader_float16_int8, > > > > > however, this is a consequence of the fact that since > > > > > c84ec70b3a72 > > > > > there is an inconsistency between what we do at the IR level > > > > > regarding > > > > > execution size of HF conversions and what the EU validator is > > > > > doing, > > > > > and from that perspective this is really fixing an > > > > > inconsistency > > > > > that > > > > > didn't exist before, and I thought we would want to address > > > > > that > > > > > sooner > > > > > rather than later and track it down to the original change > > > > > that > > > > > introduced that inconsistency so we know where this is coming > > > > > from. > > > > > > > > > > > > > The "inconsistency" between the IR's get_exec_type() and the EU > > > > validator's execution_type() has existed ever since > > > > a05b6f25bf4bfad7 > > > > removed the HF assert from get_exec_type() without actually > > > > implementing > > > > the code required to handle HF operands (which is what my > > > > commit > > > > c84ec70b3a72 did). > > > > > > I agree with the fact that since a05b6f25bf4bfad7 the validator > > > could > > > reject valid code and that had nothing to do with your patch, > > > > The validator rejected the same valid HF code since it was written, > > that > > had nothing to do with neither a05b6f25bf4bfad7 nor with my patch, > > and > > it is the real problem this patch was working around. > > > > > but the inconsistency I am talking about here, that this patch > > > fixes, > > > is the one about get_exec_type() in the IR and execution_type() > > > in > > > the > > > validator doing different things for HF instructions, which only > > > exists since your patch and which you discuss below. > > > > > > > The "inconsistency" exists ever since get_exec_type() was > > introduced > > without correct handling of HF types (even though execution_type() > > already attempted to handle it). And I disagree that it's a real > > inconsistency except due to the fact that the validator is > > incorrectly > > attempting to validate the alignment of the destination region > > according > > to a rule that doesn't apply to HF types. > > > > > > > Anyway, that was my rationale for the Fixes tag, but if you > > > > > think > > > > > this > > > > > is not useful I am happy to drop this patch and just include > > > > > it > > > > > as > > > > > part > > > > > of my series without the tag. > > > > > > > > > > > > > I'd like to see the actual regioning restrictions for HF types > > > > implemented in the EU validator as part of your series. > > > > > > Ok, let's see if we can agree on what restrictions should we > > > implement > > > then. I can implement this restriction as documented: > > > > > > "Conversion between Integer and HF (Half Float) must be DWord- > > > aligned > > > and strided by a DWord on the
Re: [Mesa-dev] [PATCH] intel/compiler: update validator to account for half-float exec type promotion
On Fri, 2019-01-25 at 12:54 -0800, Francisco Jerez wrote: > Iago Toral writes: > > > On Thu, 2019-01-24 at 11:45 -0800, Francisco Jerez wrote: > > > Iago Toral writes: > > > > > > > On Wed, 2019-01-23 at 06:03 -0800, Francisco Jerez wrote: > > > > > Iago Toral Quiroga writes: > > > > > > > > > > > Commit c84ec70b3a72 implemented execution type promotion to > > > > > > 32- > > > > > > bit > > > > > > for > > > > > > conversions involving half-float registers, which empirical > > > > > > testing > > > > > > suggested > > > > > > was required, but it did not incorporate this change into > > > > > > the > > > > > > assembly validator > > > > > > logic. This commits adds that, preventing validation errors > > > > > > like > > > > > > this: > > > > > > > > > > > > > > > > I don't think we should be validating empirical assumptions > > > > > in > > > > > the EU > > > > > validator. > > > > > > > > I am not sure I get your point, isn't c84ec70b3a72 also based > > > > on > > > > empirical testing after all? > > > > > > > > > > To some extent, but it doesn't attempt to enforce ISA > > > restrictions > > > based > > > on information obtained empirically. > > > > > > > > > > > > > mov(16) g9<4>B g3<16,8,2>HF { align1 1H }; > > > > > > ERROR: Destination stride must be equal to the ratio of the > > > > > > sizes > > > > > > of the > > > > > >execution data type to the destination type > > > > > > > > > > > > Fixes: c84ec70b3a72 "intel/fs: Promote execution type to > > > > > > 32-bit > > > > > > when any half-float conversion is needed." > > > > > > > > > > I don't think this "fixes" anything that ever worked. > > > > > > > > It is true that the code in that trace above is not something > > > > we > > > > can > > > > produce right now, because it is a conversion from HF to B and > > > > that > > > > should only happen within the context of > > > > VK_KHR_shader_float16_int8, > > > > however, this is a consequence of the fact that since > > > > c84ec70b3a72 > > > > there is an inconsistency between what we do at the IR level > > > > regarding > > > > execution size of HF conversions and what the EU validator is > > > > doing, > > > > and from that perspective this is really fixing an > > > > inconsistency > > > > that > > > > didn't exist before, and I thought we would want to address > > > > that > > > > sooner > > > > rather than later and track it down to the original change that > > > > introduced that inconsistency so we know where this is coming > > > > from. > > > > > > > > > > The "inconsistency" between the IR's get_exec_type() and the EU > > > validator's execution_type() has existed ever since > > > a05b6f25bf4bfad7 > > > removed the HF assert from get_exec_type() without actually > > > implementing > > > the code required to handle HF operands (which is what my commit > > > c84ec70b3a72 did). > > > > I agree with the fact that since a05b6f25bf4bfad7 the validator > > could > > reject valid code and that had nothing to do with your patch, > > The validator rejected the same valid HF code since it was written, > that > had nothing to do with neither a05b6f25bf4bfad7 nor with my patch, > and > it is the real problem this patch was working around. > > > but the inconsistency I am talking about here, that this patch > > fixes, > > is the one about get_exec_type() in the IR and execution_type() in > > the > > validator doing different things for HF instructions, which only > > exists since your patch and which you discuss below. > > > > The "inconsistency" exists ever since get_exec_type() was introduced > without correct handling of HF types (even though execution_type() > already attempted to handle it). And I disagree that it's a real > inconsistency except due to the fact that the validator is > incorrectly > attempting to validate the alignment of the destination region > according > to a rule that doesn't apply to HF types. > > > > > Anyway, that was my rationale for the Fixes tag, but if you > > > > think > > > > this > > > > is not useful I am happy to drop this patch and just include it > > > > as > > > > part > > > > of my series without the tag. > > > > > > > > > > I'd like to see the actual regioning restrictions for HF types > > > implemented in the EU validator as part of your series. > > > > Ok, let's see if we can agree on what restrictions should we > > implement > > then. I can implement this restriction as documented: > > > > "Conversion between Integer and HF (Half Float) must be DWord- > > aligned > > and strided by a DWord on the destination" > > > > Instead of trying to apply the general one that is currently > > breaking. > > That will fix the assertion issue. I guess my issue with it was > > that > > going by your analysis this restriction is not telling the full > > picture, this is what you had to say about this restriction: > > > > "I have a feeling that the reason for this may be that the 16-bit > > pipeline lacks the ability to handle
Re: [Mesa-dev] [PATCH] intel/compiler: update validator to account for half-float exec type promotion
On Fri, 2019-01-25 at 12:54 -0800, Francisco Jerez wrote: > Iago Toral writes: > > > On Thu, 2019-01-24 at 11:45 -0800, Francisco Jerez wrote: > > > Iago Toral writes: > > > > > > > On Wed, 2019-01-23 at 06:03 -0800, Francisco Jerez wrote: > > > > > Iago Toral Quiroga writes: (...) > > > I'd like to see the actual regioning restrictions for HF types > > > implemented in the EU validator as part of your series. > > > > Ok, let's see if we can agree on what restrictions should we > > implement > > then. I can implement this restriction as documented: > > > > "Conversion between Integer and HF (Half Float) must be DWord- > > aligned > > and strided by a DWord on the destination" > > > > Instead of trying to apply the general one that is currently > > breaking. > > That will fix the assertion issue. I guess my issue with it was > > that > > going by your analysis this restriction is not telling the full > > picture, this is what you had to say about this restriction: > > > > "I have a feeling that the reason for this may be that the 16-bit > > pipeline lacks the ability to handle conversions from or to half- > > float, > > so the execution type is implicitly promoted to the matching > > (integer > > or floating-point) 32-bit type where any HF conversion would be > > needed. And on those the usual alignment restriction of the > > destination to a larger execution type applies." > > > > But I guess your point is that we can ignore these details at the > > validator level and just focus on validating strictly what the PRM > > says. Fair enough. > > > > Unfortunatley, you also mentioned that this restriction *seems* to > > be > > overriden by this one on all platforms but BDW (even though the > > both > > are listed, at least I see both in my SKL docs): > > > > "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." > > > > I'd implement this one since it seems like the most specific, except > on > BDW where only the previous (almost equivalent) restriction seems to > apply. So we apply the one about relaxed alignment on all platforms but BDW, and on BDW we apply the other one. Ok. > There are shitloads of other restrictions we're missing for HF > types BTW, check out the section "Special Requirements for Handling > Mixed Mode Float Operations" of the hardware spec. Yes, I'll go through those in my series too, but I want to have a clear understanding about the others since they are the ones that triggered this discussion. > > Which you discussed didn't make sense to you and I agreed, if only > > because my own experiments suggested that the implications that one > > would derive from a strict interpretation of this (that packed 16- > > bit > > data is not supported) are not quite true going by the fact that we > > are > > emitting packed instructions on all platforms without any > > indication > > that this doesn't work. So what should we do about this one? If we > > decide that we want to implement this then we have to re-think the > > implementation we have. > > > > The implication on packed HF instructions is likely accidental, I > don't > think we should enforce the rule except for conversion operations. Ok, I'll implement this only for conversions then. > > Should we just validate the one about the integer/float > > conversions? > > Should we do that only on BDW or on all platforms? > > > > > I don't see the > > > need to introduce any empirical assumptions into the EU > > > validator's > > > execution_type() in order to achieve that, even if that means > > > that > > > get_exec_type() and execution_type() don't do the exact same > > > calculation > > > -- What you call an inconsistency is the consequence of > > > execution_type() > > > being the hardware spec's opinion on what the execution type is, > > > which > > > we assume is what we need to use while enforcing a regioning > > > restriction > > > that refers to the execution type of the instruction. > > > > Mmm... I guess my question is: is the hardware really assuming that > > execution type in all cases that involve HF instructions? What I > > got > > from your analysis of the whole situation is that the hardware is > > actually promoting the execution size to 32-bit in some cases, and > > if > > that is the case, then does it make sense to implement code in the > > validator that ignores that fact? > > If the hardware spec chose to ignore that fact and instead gave us a > different set of regioning restrictions for us to use on HF > restrictions > that don't rely on the definition of execution type, we should > probably > have the validator match the hardware spec literally. > Ok, my initial understanding was that the
Re: [Mesa-dev] [PATCH] intel/compiler: update validator to account for half-float exec type promotion
Iago Toral writes: > On Thu, 2019-01-24 at 11:45 -0800, Francisco Jerez wrote: >> Iago Toral writes: >> >> > On Wed, 2019-01-23 at 06:03 -0800, Francisco Jerez wrote: >> > > Iago Toral Quiroga writes: >> > > >> > > > Commit c84ec70b3a72 implemented execution type promotion to 32- >> > > > bit >> > > > for >> > > > conversions involving half-float registers, which empirical >> > > > testing >> > > > suggested >> > > > was required, but it did not incorporate this change into the >> > > > assembly validator >> > > > logic. This commits adds that, preventing validation errors >> > > > like >> > > > this: >> > > > >> > > >> > > I don't think we should be validating empirical assumptions in >> > > the EU >> > > validator. >> > >> > I am not sure I get your point, isn't c84ec70b3a72 also based on >> > empirical testing after all? >> > >> >> To some extent, but it doesn't attempt to enforce ISA restrictions >> based >> on information obtained empirically. >> >> > >> > > > mov(16) g9<4>B g3<16,8,2>HF { align1 1H }; >> > > > ERROR: Destination stride must be equal to the ratio of the >> > > > sizes >> > > > of the >> > > >execution data type to the destination type >> > > > >> > > > Fixes: c84ec70b3a72 "intel/fs: Promote execution type to 32-bit >> > > > when any half-float conversion is needed." >> > > >> > > I don't think this "fixes" anything that ever worked. >> > >> > It is true that the code in that trace above is not something we >> > can >> > produce right now, because it is a conversion from HF to B and that >> > should only happen within the context of >> > VK_KHR_shader_float16_int8, >> > however, this is a consequence of the fact that since c84ec70b3a72 >> > there is an inconsistency between what we do at the IR level >> > regarding >> > execution size of HF conversions and what the EU validator is >> > doing, >> > and from that perspective this is really fixing an inconsistency >> > that >> > didn't exist before, and I thought we would want to address that >> > sooner >> > rather than later and track it down to the original change that >> > introduced that inconsistency so we know where this is coming from. >> > >> >> The "inconsistency" between the IR's get_exec_type() and the EU >> validator's execution_type() has existed ever since a05b6f25bf4bfad7 >> removed the HF assert from get_exec_type() without actually >> implementing >> the code required to handle HF operands (which is what my commit >> c84ec70b3a72 did). > > I agree with the fact that since a05b6f25bf4bfad7 the validator could > reject valid code and that had nothing to do with your patch, The validator rejected the same valid HF code since it was written, that had nothing to do with neither a05b6f25bf4bfad7 nor with my patch, and it is the real problem this patch was working around. > but the inconsistency I am talking about here, that this patch fixes, > is the one about get_exec_type() in the IR and execution_type() in the > validator doing different things for HF instructions, which only > exists since your patch and which you discuss below. > The "inconsistency" exists ever since get_exec_type() was introduced without correct handling of HF types (even though execution_type() already attempted to handle it). And I disagree that it's a real inconsistency except due to the fact that the validator is incorrectly attempting to validate the alignment of the destination region according to a rule that doesn't apply to HF types. >> > Anyway, that was my rationale for the Fixes tag, but if you think >> > this >> > is not useful I am happy to drop this patch and just include it as >> > part >> > of my series without the tag. >> > >> >> I'd like to see the actual regioning restrictions for HF types >> implemented in the EU validator as part of your series. > > Ok, let's see if we can agree on what restrictions should we implement > then. I can implement this restriction as documented: > > "Conversion between Integer and HF (Half Float) must be DWord-aligned > and strided by a DWord on the destination" > > Instead of trying to apply the general one that is currently breaking. > That will fix the assertion issue. I guess my issue with it was that > going by your analysis this restriction is not telling the full > picture, this is what you had to say about this restriction: > > "I have a feeling that the reason for this may be that the 16-bit > pipeline lacks the ability to handle conversions from or to half-float, > so the execution type is implicitly promoted to the matching (integer > or floating-point) 32-bit type where any HF conversion would be > needed. And on those the usual alignment restriction of the > destination to a larger execution type applies." > > But I guess your point is that we can ignore these details at the > validator level and just focus on validating strictly what the PRM > says. Fair enough. > > Unfortunatley, you also mentioned that this restriction *seems* to be > overriden by
Re: [Mesa-dev] [PATCH] intel/compiler: update validator to account for half-float exec type promotion
On Thu, 2019-01-24 at 10:22 -0800, Matt Turner wrote: > On Wed, Jan 23, 2019 at 4:18 AM Iago Toral Quiroga > wrote: > > > > Commit c84ec70b3a72 implemented execution type promotion to 32-bit > > for > > conversions involving half-float registers, which empirical testing > > suggested > > was required, but it did not incorporate this change into the > > assembly validator > > logic. This commits adds that, preventing validation errors like > > this: > > > > mov(16) g9<4>B g3<16,8,2>HF { align1 1H }; > > ERROR: Destination stride must be equal to the ratio of the sizes > > of the > >execution data type to the destination type > > > > Fixes: c84ec70b3a72 "intel/fs: Promote execution type to 32-bit > > when any half-float conversion is needed." > > --- > > src/intel/compiler/brw_eu_validate.c | 27 ++ > > - > > New rule: New restrictions (or relaxations) may not be added to > brw_eu_validate.c without accompanying unit tests. I'll send a patch > to add a comment to brw_eu_validate.c saying as much. > > Rationale: the reason I wrote brw_eu_validate.c was because I wasted > a > week debugging an issue where fulsim not only failed to inform me > that > one instruction was invalid but also incorrectly told me that one > correct instruction *was* invalid. I would have been better off > without such a tool. > > If the EU validator loses people's trust, then it's useless, but if > it > is incorrect it's worse than useless. Makes sense to me. Iago ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] intel/compiler: update validator to account for half-float exec type promotion
On Thu, 2019-01-24 at 11:45 -0800, Francisco Jerez wrote: > Iago Toral writes: > > > On Wed, 2019-01-23 at 06:03 -0800, Francisco Jerez wrote: > > > Iago Toral Quiroga writes: > > > > > > > Commit c84ec70b3a72 implemented execution type promotion to 32- > > > > bit > > > > for > > > > conversions involving half-float registers, which empirical > > > > testing > > > > suggested > > > > was required, but it did not incorporate this change into the > > > > assembly validator > > > > logic. This commits adds that, preventing validation errors > > > > like > > > > this: > > > > > > > > > > I don't think we should be validating empirical assumptions in > > > the EU > > > validator. > > > > I am not sure I get your point, isn't c84ec70b3a72 also based on > > empirical testing after all? > > > > To some extent, but it doesn't attempt to enforce ISA restrictions > based > on information obtained empirically. > > > > > > > mov(16) g9<4>B g3<16,8,2>HF { align1 1H }; > > > > ERROR: Destination stride must be equal to the ratio of the > > > > sizes > > > > of the > > > >execution data type to the destination type > > > > > > > > Fixes: c84ec70b3a72 "intel/fs: Promote execution type to 32-bit > > > > when any half-float conversion is needed." > > > > > > I don't think this "fixes" anything that ever worked. > > > > It is true that the code in that trace above is not something we > > can > > produce right now, because it is a conversion from HF to B and that > > should only happen within the context of > > VK_KHR_shader_float16_int8, > > however, this is a consequence of the fact that since c84ec70b3a72 > > there is an inconsistency between what we do at the IR level > > regarding > > execution size of HF conversions and what the EU validator is > > doing, > > and from that perspective this is really fixing an inconsistency > > that > > didn't exist before, and I thought we would want to address that > > sooner > > rather than later and track it down to the original change that > > introduced that inconsistency so we know where this is coming from. > > > > The "inconsistency" between the IR's get_exec_type() and the EU > validator's execution_type() has existed ever since a05b6f25bf4bfad7 > removed the HF assert from get_exec_type() without actually > implementing > the code required to handle HF operands (which is what my commit > c84ec70b3a72 did). I agree with the fact that since a05b6f25bf4bfad7 the validator could reject valid code and that had nothing to do with your patch, but the inconsistency I am talking about here, that this patch fixes, is the one about get_exec_type() in the IR and execution_type() in the validator doing different things for HF instructions, which only exists since your patch and which you discuss below. > > Anyway, that was my rationale for the Fixes tag, but if you think > > this > > is not useful I am happy to drop this patch and just include it as > > part > > of my series without the tag. > > > > I'd like to see the actual regioning restrictions for HF types > implemented in the EU validator as part of your series. Ok, let's see if we can agree on what restrictions should we implement then. I can implement this restriction as documented: "Conversion between Integer and HF (Half Float) must be DWord-aligned and strided by a DWord on the destination" Instead of trying to apply the general one that is currently breaking. That will fix the assertion issue. I guess my issue with it was that going by your analysis this restriction is not telling the full picture, this is what you had to say about this restriction: "I have a feeling that the reason for this may be that the 16-bit pipeline lacks the ability to handle conversions from or to half-float, so the execution type is implicitly promoted to the matching (integer or floating-point) 32-bit type where any HF conversion would be needed. And on those the usual alignment restriction of the destination to a larger execution type applies." But I guess your point is that we can ignore these details at the validator level and just focus on validating strictly what the PRM says. Fair enough. Unfortunatley, you also mentioned that this restriction *seems* to be overriden by this one on all platforms but BDW (even though the both are listed, at least I see both in my SKL docs): "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." Which you discussed didn't make sense to you and I agreed, if only because my own experiments suggested that the implications that one would derive from a strict interpretation of this (that packed 16-bit data is not supported) are not quite true going by the fact that we are emitting packed
Re: [Mesa-dev] [PATCH] intel/compiler: update validator to account for half-float exec type promotion
Iago Toral writes: > On Wed, 2019-01-23 at 06:03 -0800, Francisco Jerez wrote: >> Iago Toral Quiroga writes: >> >> > Commit c84ec70b3a72 implemented execution type promotion to 32-bit >> > for >> > conversions involving half-float registers, which empirical testing >> > suggested >> > was required, but it did not incorporate this change into the >> > assembly validator >> > logic. This commits adds that, preventing validation errors like >> > this: >> > >> >> I don't think we should be validating empirical assumptions in the EU >> validator. > > I am not sure I get your point, isn't c84ec70b3a72 also based on > empirical testing after all? > To some extent, but it doesn't attempt to enforce ISA restrictions based on information obtained empirically. > >> > mov(16) g9<4>B g3<16,8,2>HF { align1 1H }; >> > ERROR: Destination stride must be equal to the ratio of the sizes >> > of the >> >execution data type to the destination type >> > >> > Fixes: c84ec70b3a72 "intel/fs: Promote execution type to 32-bit >> > when any half-float conversion is needed." >> >> I don't think this "fixes" anything that ever worked. > > It is true that the code in that trace above is not something we can > produce right now, because it is a conversion from HF to B and that > should only happen within the context of VK_KHR_shader_float16_int8, > however, this is a consequence of the fact that since c84ec70b3a72 > there is an inconsistency between what we do at the IR level regarding > execution size of HF conversions and what the EU validator is doing, > and from that perspective this is really fixing an inconsistency that > didn't exist before, and I thought we would want to address that sooner > rather than later and track it down to the original change that > introduced that inconsistency so we know where this is coming from. > The "inconsistency" between the IR's get_exec_type() and the EU validator's execution_type() has existed ever since a05b6f25bf4bfad7 removed the HF assert from get_exec_type() without actually implementing the code required to handle HF operands (which is what my commit c84ec70b3a72 did). > Anyway, that was my rationale for the Fixes tag, but if you think this > is not useful I am happy to drop this patch and just include it as part > of my series without the tag. > I'd like to see the actual regioning restrictions for HF types implemented in the EU validator as part of your series. I don't see the need to introduce any empirical assumptions into the EU validator's execution_type() in order to achieve that, even if that means that get_exec_type() and execution_type() don't do the exact same calculation -- What you call an inconsistency is the consequence of execution_type() being the hardware spec's opinion on what the execution type is, which we assume is what we need to use while enforcing a regioning restriction that refers to the execution type of the instruction. >> The validator is >> still missing an implementation of the quirky HF restrictions, and it >> wasn't the purpose of c84ec70b3a72 to do such a thing. > > While this is true in general, the EU validator does consider the > execution type of the instruction to validate general rules such as the > one I mentioned in the commit message in this patch. And that part of > the validator is inconsistent with c84ec70b3a72. That part of the validator was also inconsistent with the code generated by your original VK_KHR_shader_float16_int8 series even before I committed c84ec70b3a72. The reason is that it is trying to validate a restriction that rejects working code, because the "general" rule it's trying to enforce isn't supposed to apply to instructions with HF operands, which is the real bug. > In fact, the EU validator is accounting for execution size promotion > of HF instructions to 32-bit in SKL+ and CHV only, for conversions > from HF->F and mixed float mode instructions... which is part of what > c84ec70b3a72 addresses at the IR level, which it actually does for all > hardware platforms and in more cases. > I'm fine with fixing execution_type() to do the right thing in more cases and platforms, but I don't think that should involve making empirical assumptions in the validator. >> You *should* >> definitely implement those restrictions (as they're stated in the >> hardware spec, without empirical assumptions) in the validator as >> part >> of your VK_KHR_shader_float16_int8 series, > > Again, I am not sure what you mean by "without empirical assumptions". I was just paraphrasing your comment. If you feel the need to write a comment justifying the existence of some code in the validator based on empirical testing, it probably doesn't belong in the validator. > According to the commit message in c84ec70b3a72 "the docs are fairly > imcomplete and inconsistent" and you explained here that you had to do > some experimentation of your own using the simulator (where you found > its results to also be
Re: [Mesa-dev] [PATCH] intel/compiler: update validator to account for half-float exec type promotion
On Wed, Jan 23, 2019 at 6:03 AM Francisco Jerez wrote: > > Iago Toral Quiroga writes: > > > Commit c84ec70b3a72 implemented execution type promotion to 32-bit for > > conversions involving half-float registers, which empirical testing > > suggested > > was required, but it did not incorporate this change into the assembly > > validator > > logic. This commits adds that, preventing validation errors like this: > > > > I don't think we should be validating empirical assumptions in the EU > validator. I kind of agree. I don't really know what we should do though. I guess it's better to err on the side of caution in the EU validator and only check restrictions that have documentation. Is that your thinking? Many instructions can only take certain conditional modifiers. XOR is documented to only take .z/.nz. However we emit XOR with a .l conditional mod for nir_op_imod. It works, and we think the documentation is incomplete. Separately it describes how the conditional modifiers operate, and .l only reads the high bit of the result so it makes sense that XOR with .l should work like we see that it does. So, (1) empirically it works, (2) the documentation says it's not allowed, but (3) there's a plausible explanation that the documentation is wrong. What should we do if we implement the conditional modifier checks in the validator? ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] intel/compiler: update validator to account for half-float exec type promotion
On Wed, Jan 23, 2019 at 4:18 AM Iago Toral Quiroga wrote: > > Commit c84ec70b3a72 implemented execution type promotion to 32-bit for > conversions involving half-float registers, which empirical testing suggested > was required, but it did not incorporate this change into the assembly > validator > logic. This commits adds that, preventing validation errors like this: > > mov(16) g9<4>B g3<16,8,2>HF { align1 1H }; > ERROR: Destination stride must be equal to the ratio of the sizes of the >execution data type to the destination type > > Fixes: c84ec70b3a72 "intel/fs: Promote execution type to 32-bit when any > half-float conversion is needed." > --- > src/intel/compiler/brw_eu_validate.c | 27 ++- New rule: New restrictions (or relaxations) may not be added to brw_eu_validate.c without accompanying unit tests. I'll send a patch to add a comment to brw_eu_validate.c saying as much. Rationale: the reason I wrote brw_eu_validate.c was because I wasted a week debugging an issue where fulsim not only failed to inform me that one instruction was invalid but also incorrectly told me that one correct instruction *was* invalid. I would have been better off without such a tool. If the EU validator loses people's trust, then it's useless, but if it is incorrect it's worse than useless. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] intel/compiler: update validator to account for half-float exec type promotion
On Wed, 2019-01-23 at 06:03 -0800, Francisco Jerez wrote: > Iago Toral Quiroga writes: > > > Commit c84ec70b3a72 implemented execution type promotion to 32-bit > > for > > conversions involving half-float registers, which empirical testing > > suggested > > was required, but it did not incorporate this change into the > > assembly validator > > logic. This commits adds that, preventing validation errors like > > this: > > > > I don't think we should be validating empirical assumptions in the EU > validator. I am not sure I get your point, isn't c84ec70b3a72 also based on empirical testing after all? > > mov(16) g9<4>B g3<16,8,2>HF { align1 1H }; > > ERROR: Destination stride must be equal to the ratio of the sizes > > of the > >execution data type to the destination type > > > > Fixes: c84ec70b3a72 "intel/fs: Promote execution type to 32-bit > > when any half-float conversion is needed." > > I don't think this "fixes" anything that ever worked. It is true that the code in that trace above is not something we can produce right now, because it is a conversion from HF to B and that should only happen within the context of VK_KHR_shader_float16_int8, however, this is a consequence of the fact that since c84ec70b3a72 there is an inconsistency between what we do at the IR level regarding execution size of HF conversions and what the EU validator is doing, and from that perspective this is really fixing an inconsistency that didn't exist before, and I thought we would want to address that sooner rather than later and track it down to the original change that introduced that inconsistency so we know where this is coming from. Anyway, that was my rationale for the Fixes tag, but if you think this is not useful I am happy to drop this patch and just include it as part of my series without the tag. > The validator is > still missing an implementation of the quirky HF restrictions, and it > wasn't the purpose of c84ec70b3a72 to do such a thing. While this is true in general, the EU validator does consider the execution type of the instruction to validate general rules such as the one I mentioned in the commit message in this patch. And that part of the validator is inconsistent with c84ec70b3a72. In fact, the EU validator is accounting for execution size promotion of HF instructions to 32-bit in SKL+ and CHV only, for conversions from HF->F and mixed float mode instructions... which is part of what c84ec70b3a72 addresses at the IR level, which it actually does for all hardware platforms and in more cases. > You *should* > definitely implement those restrictions (as they're stated in the > hardware spec, without empirical assumptions) in the validator as > part > of your VK_KHR_shader_float16_int8 series, Again, I am not sure what you mean by "without empirical assumptions". According to the commit message in c84ec70b3a72 "the docs are fairly imcomplete and inconsistent" and you explained here that you had to do some experimentation of your own using the simulator (where you found its results to also be inconsistent with the hardware docs) to try and guess what is happening. That's why I was using the term "empirical" here, since the hardware docs alone aren't clear or correct enough to understand what is really happening, when and in what platforms. Anyway, if you don't like the term "empirical" to refer to all this, that's fine by me, but what I need to know is if we agree that we need to implement the same type promotion rules in the validator that we are implementing in the IR, indepedently of whether refer to them as "based on empirical testing" or not. > if anything because currently > it will reject working code that uses HF types. Just for the sake of clarity, since that sentence above could be interpreted as if all HF code would be rejected: we have been using HF types since we landed VK_KHR_16bit_storage, which includes conversions between HF and F and the EU validator is not complaining about any of that. It is currently a problem only with conversions to smaller types (so only conversions to Byte) because that's where we check for that restriction on the stride, which is based on the execution type of the instruction. > > > --- > > src/intel/compiler/brw_eu_validate.c | 27 ++ > > - > > 1 file changed, 14 insertions(+), 13 deletions(-) > > > > diff --git a/src/intel/compiler/brw_eu_validate.c > > b/src/intel/compiler/brw_eu_validate.c > > index a25010b225c..3bb37677672 100644 > > --- a/src/intel/compiler/brw_eu_validate.c > > +++ b/src/intel/compiler/brw_eu_validate.c > > @@ -325,17 +325,20 @@ execution_type(const struct gen_device_info > > *devinfo, const brw_inst *inst) > > unsigned num_sources = num_sources_from_inst(devinfo, inst); > > enum brw_reg_type src0_exec_type, src1_exec_type; > > > > - /* Execution data type is independent of destination data type, > > except in > > -* mixed F/HF instructions on CHV and SKL+. > >
Re: [Mesa-dev] [PATCH] intel/compiler: update validator to account for half-float exec type promotion
Iago Toral Quiroga writes: > Commit c84ec70b3a72 implemented execution type promotion to 32-bit for > conversions involving half-float registers, which empirical testing suggested > was required, but it did not incorporate this change into the assembly > validator > logic. This commits adds that, preventing validation errors like this: > I don't think we should be validating empirical assumptions in the EU validator. > mov(16) g9<4>B g3<16,8,2>HF { align1 1H }; > ERROR: Destination stride must be equal to the ratio of the sizes of the >execution data type to the destination type > > Fixes: c84ec70b3a72 "intel/fs: Promote execution type to 32-bit when any > half-float conversion is needed." I don't think this "fixes" anything that ever worked. The validator is still missing an implementation of the quirky HF restrictions, and it wasn't the purpose of c84ec70b3a72 to do such a thing. You *should* definitely implement those restrictions (as they're stated in the hardware spec, without empirical assumptions) in the validator as part of your VK_KHR_shader_float16_int8 series, if anything because currently it will reject working code that uses HF types. > --- > src/intel/compiler/brw_eu_validate.c | 27 ++- > 1 file changed, 14 insertions(+), 13 deletions(-) > > diff --git a/src/intel/compiler/brw_eu_validate.c > b/src/intel/compiler/brw_eu_validate.c > index a25010b225c..3bb37677672 100644 > --- a/src/intel/compiler/brw_eu_validate.c > +++ b/src/intel/compiler/brw_eu_validate.c > @@ -325,17 +325,20 @@ execution_type(const struct gen_device_info *devinfo, > const brw_inst *inst) > unsigned num_sources = num_sources_from_inst(devinfo, inst); > enum brw_reg_type src0_exec_type, src1_exec_type; > > - /* Execution data type is independent of destination data type, except in > -* mixed F/HF instructions on CHV and SKL+. > + /* Empirical testing suggests that type conversions involving half-float > +* promote execution type to 32-bit. See get_exec_type() in brw_ir_fs.h. > */ > enum brw_reg_type dst_exec_type = brw_inst_dst_type(devinfo, inst); > > src0_exec_type = execution_type_for_type(brw_inst_src0_type(devinfo, > inst)); > if (num_sources == 1) { > - if ((devinfo->gen >= 9 || devinfo->is_cherryview) && > - src0_exec_type == BRW_REGISTER_TYPE_HF) { > - return dst_exec_type; > + if (type_sz(src0_exec_type) == 2 && dst_exec_type != src0_exec_type) { > + if (src0_exec_type == BRW_REGISTER_TYPE_HF) > +return BRW_REGISTER_TYPE_F; > + else if (dst_exec_type == BRW_REGISTER_TYPE_HF) > +return BRW_REGISTER_TYPE_D; >} > + >return src0_exec_type; > } > > @@ -367,14 +370,12 @@ execution_type(const struct gen_device_info *devinfo, > const brw_inst *inst) > src1_exec_type == BRW_REGISTER_TYPE_DF) >return BRW_REGISTER_TYPE_DF; > > - if (devinfo->gen >= 9 || devinfo->is_cherryview) { > - if (dst_exec_type == BRW_REGISTER_TYPE_F || > - src0_exec_type == BRW_REGISTER_TYPE_F || > - src1_exec_type == BRW_REGISTER_TYPE_F) { > - return BRW_REGISTER_TYPE_F; > - } else { > - return BRW_REGISTER_TYPE_HF; > - } > + if (dst_exec_type == BRW_REGISTER_TYPE_F || > + src0_exec_type == BRW_REGISTER_TYPE_F || > + src1_exec_type == BRW_REGISTER_TYPE_F) { > + return BRW_REGISTER_TYPE_F; > + } else { > + return BRW_REGISTER_TYPE_HF; > } > > assert(src0_exec_type == BRW_REGISTER_TYPE_F); > -- > 2.17.1 signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] intel/compiler: update validator to account for half-float exec type promotion
Commit c84ec70b3a72 implemented execution type promotion to 32-bit for conversions involving half-float registers, which empirical testing suggested was required, but it did not incorporate this change into the assembly validator logic. This commits adds that, preventing validation errors like this: mov(16) g9<4>B g3<16,8,2>HF { align1 1H }; ERROR: Destination stride must be equal to the ratio of the sizes of the execution data type to the destination type Fixes: c84ec70b3a72 "intel/fs: Promote execution type to 32-bit when any half-float conversion is needed." --- src/intel/compiler/brw_eu_validate.c | 27 ++- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/src/intel/compiler/brw_eu_validate.c b/src/intel/compiler/brw_eu_validate.c index a25010b225c..3bb37677672 100644 --- a/src/intel/compiler/brw_eu_validate.c +++ b/src/intel/compiler/brw_eu_validate.c @@ -325,17 +325,20 @@ execution_type(const struct gen_device_info *devinfo, const brw_inst *inst) unsigned num_sources = num_sources_from_inst(devinfo, inst); enum brw_reg_type src0_exec_type, src1_exec_type; - /* Execution data type is independent of destination data type, except in -* mixed F/HF instructions on CHV and SKL+. + /* Empirical testing suggests that type conversions involving half-float +* promote execution type to 32-bit. See get_exec_type() in brw_ir_fs.h. */ enum brw_reg_type dst_exec_type = brw_inst_dst_type(devinfo, inst); src0_exec_type = execution_type_for_type(brw_inst_src0_type(devinfo, inst)); if (num_sources == 1) { - if ((devinfo->gen >= 9 || devinfo->is_cherryview) && - src0_exec_type == BRW_REGISTER_TYPE_HF) { - return dst_exec_type; + if (type_sz(src0_exec_type) == 2 && dst_exec_type != src0_exec_type) { + if (src0_exec_type == BRW_REGISTER_TYPE_HF) +return BRW_REGISTER_TYPE_F; + else if (dst_exec_type == BRW_REGISTER_TYPE_HF) +return BRW_REGISTER_TYPE_D; } + return src0_exec_type; } @@ -367,14 +370,12 @@ execution_type(const struct gen_device_info *devinfo, const brw_inst *inst) src1_exec_type == BRW_REGISTER_TYPE_DF) return BRW_REGISTER_TYPE_DF; - if (devinfo->gen >= 9 || devinfo->is_cherryview) { - if (dst_exec_type == BRW_REGISTER_TYPE_F || - src0_exec_type == BRW_REGISTER_TYPE_F || - src1_exec_type == BRW_REGISTER_TYPE_F) { - return BRW_REGISTER_TYPE_F; - } else { - return BRW_REGISTER_TYPE_HF; - } + if (dst_exec_type == BRW_REGISTER_TYPE_F || + src0_exec_type == BRW_REGISTER_TYPE_F || + src1_exec_type == BRW_REGISTER_TYPE_F) { + return BRW_REGISTER_TYPE_F; + } else { + return BRW_REGISTER_TYPE_HF; } assert(src0_exec_type == BRW_REGISTER_TYPE_F); -- 2.17.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev