[Mesa-dev] [Mesa-stable] [PATCH] i965/fs: Don't emit SEL instructions for type-converting MOVs.

[Samuel Iglesias Gonsálvez]

On Mon, Mar 27, 2017 at 06:30:19AM +0200, Samuel Iglesias Gonsálvez wrote:
> On Fri, Mar 24, 2017 at 12:39:38PM -0700, Francisco Jerez wrote:
> > Matt Turner <mattst88 at gmail.com> writes:
> > 
> > > On Fri, Mar 24, 2017 at 12:06 AM, Francisco Jerez <currojerez at riseup.net> wrote:
> > >> Samuel Iglesias Gonsálvez <siglesias at igalia.com> writes:
> > >>
> > >>> On Thu, 2017-03-23 at 13:50 -0700, Matt Turner wrote:
> > >>>> SEL can only convert between a few integer types, which we basically
> > >>>> never do.
> > >>>>
> > >>>> Fixes fs/vs-double-uniform-array-direct-indirect-non-uniform-control-
> > >>>> flow
> > >>>> Cc: mesa-stable at lists.freedesktop.org
> > >>>
> > >>> I sent a similar but wrong patch (taking only into account the type
> > >>> size) some time ago, but after discussing it with Curro, the solution
> > >>> was to fix it inside d2x pass. This is what this patch "i965/fs:
> > >>> generalize the legalization d2x pass" does.
> > >>>
> > >>> I am still working on improving that patch but I expect to have
> > >>> something soon. If you prefer to land this now, please add my R-b but
> > >>> you probably want to discuss it with Curro before:
> > >>>
> > >>> Reviewed-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
> > >>>
> > >>
> > >> Samuel's d2x patch has the advantage that it will allow the SEL peephole
> > >> to replace control flow with SEL instructions even where there is a type
> > >> conversion.  That said this patch shouldn't hurt in mesa-stable in the
> > >> meantime if we remember to revert it in master when Samuel's patch
> > >> lands.  Patch is:
> > >>
> > >> Acked-by: Francisco Jerez <currojerez at riseup.net>
> > >
> > > Oh, I didn't realize that pass was going to handle instructions not
> > > operating on DF types. That's surprising given its name.
> > 
> > Yeah, he's also sent a patch to rename it.
> > 
> > >
> > > To confirm: with that pass in place it should be save to do a
> > > type-converting SEL (on, say, integer sources and a float destination)
> > > in the IR?
> > >
> > 
> > Yes, in principle it should be safe for the optimizer to introduce type
> > conversions of any kind, although at this point the lower_conversions
> > pass only handles MOV, MOV_INDIRECT and SEL opcodes it should be
> > straightforward to extend it to handle the type conversion restrictions
> > of any instruction.
> > 
> > > If that's the case, I'll delay committing this patch until lower_d2x
> > > is committed, so that we don't have to remember to revert my patch,
> > > and there's no chance of bugs being fixed on the stable branch but not
> > > in master.
> > 
> > I guess if you commit it to master already there's no chance of it not
> > getting fixed in master, the only concern is that we'll end up with both
> > fixes checked in forever.  Samuel, would you mind including a revert of
> > this change as PATCH 7.5 of your FP64 series?
> 
> Sure, I will include it.

Forgot to say that I will add Matt's and yours R-b to that revert, so we don't
need to wait for them.

Sam

> 
> Thanks!
> 
> Sam
> 
> 
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170327/33ffb42f/attachment.sig>