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

[Samuel Iglesias Gonsálvez]

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.

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/83ea6f2a/attachment.sig>