Re: Moving code around, post classic

[Timothy Arceri]

On Tue, 2021-12-07 at 08:19 -0500, Alyssa Rosenzweig wrote:
> > 
> 
> > 2. Move src/compiler/glsl into src/gallium/frontends/mesa as well
> > 
> >     Given that there are now no? drivers that use GLSL-IR directly,
> > it
> >     might make sense to move the glsl compiler into the mesa
> >     state_tracker, and just have that lower to TGSI or NIR, and
> > treat
> >     GLSL-IR as an implementation detail of the OpenGL frontend.
> 
> It would be an ack from, but...
> 
> >     Unfortunately, there are a lot of code outside of glsl that
> > uses the
> >     linked list implementation in the glsl compiler, and not the on
> > in
> >     util.
> 
> If it were just linked lists, I'd say someone should write the
> Coccinelle to transform the tree to use the one in util and call it a
> day. It's a bit more complicated though, NIR depends on GLSL types.
> Though that could probably continue to live in its current location
> even
> if glsl moves? Might breed confusion.

I agree moving this is more trouble than its worth IMO, its easy to
understand why its in the compiler dir, and its right next to all the
common code it shares with nir, shader_info, glsl_types, etc.

Its also worth noting that the state tracker isn't the only potental
user as the glsl compile can still be build as a standalone executable.
My prefrence is for this code to stay where it is.

Re: [Mesa-dev] [RFC] Concrete proposal to split classic

[Timothy Arceri]

On 6/16/21 1:16 PM, Jason Ekstrand wrote:


On Tue, Jun 15, 2021 at 8:46 PM Timothy Arceri  wrote:

On 6/16/21 11:03 AM, Jason Ekstrand wrote:

I'm bringing this up via e-mail so it gets a wider audience. Given how will 
crocus is working at this point, is like to propose we hold off for about three 
more releases before we drop classic. This next release, 21.2, we'll have 
crocus as an option with i965 as the default. There will also be a 
-Dprefer-crocus meson options so distros or individuals can attempt to flip it 
on. The release after that, 21.3, we'll keep i965 in the tree but have crocus 
be the default (assuming things are going well.) Some time in 2022, probably 
after the 22.2 release or so, we'll delete classic.

Why wait so long? Well, it just landed and we don't have a Cherryview story yet 
so I'm hesitant to make it the default too quickly. Even if it were the default 
in 21.2, it's already too late, likely, to hit the fall 2021 distro release 
cycle. If we flip it to the default before the end of the year, that'll get 
crocus into spring distros. This is good because 22.04 is an Ubuntu LTS release 
and I think they'd rather bump crocus versions to fix bugs than backport on top 
of i965. But that's really fort Ubuntu to decide. In any case, we won't see 
broad-spread usage and the flood of bug reports until next spring so we may 
want to wait until then to stay deleting code.

If we wanted to accelerate things, one option, once we're ready, would be to 
ask the person who manages the oibaf PPA to switch to crocus early. That may 
get some early adopters on board.

Thoughts?

I though the idea was to put everything in a classic branch and let distros run 
"classic" hardware from that. What happens after 3 releases does i965 still go 
to the classic branch with the other classic drivers? If so is it really worth waiting 
just because Ubuntu might have to back-port a bug fix?

Yeah, that was the idea.  However, with crocus in good shape and Emma
Anholt working on i915g, it may be that the actual answer is we just
throw away the classic drivers and the only thing you really need the
old branch for is r200 and ancient nouveau.
Fair enough. I don't really have any stake in these drivers, but I am 
eager to get to work on clean ups once we drop the classic drivers. I 
would be disappointed if we were forced to wait another year just to 
have the Intel drivers kept in the classic branch anyway. Going on past 
experience we can pretty much guarantee that someone will at least ask 
we keep them. Anyway up to you guys I guess.

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [RFC] Concrete proposal to split classic

[Timothy Arceri]

On 6/16/21 11:03 AM, Jason Ekstrand wrote:

I'm bringing this up via e-mail so it gets a wider audience. Given how 
will crocus is working at this point, is like to propose we hold off 
for about three more releases before we drop classic. This next 
release, 21.2, we'll have crocus as an option with i965 as the 
default. There will also be a -Dprefer-crocus meson options so distros 
or individuals can attempt to flip it on. The release after that, 
21.3, we'll keep i965 in the tree but have crocus be the default 
(assuming things are going well.) Some time in 2022, probably after 
the 22.2 release or so, we'll delete classic.


Why wait so long? Well, it just landed and we don't have a Cherryview 
story yet so I'm hesitant to make it the default too quickly. Even if 
it were the default in 21.2, it's already too late, likely, to hit the 
fall 2021 distro release cycle. If we flip it to the default before 
the end of the year, that'll get crocus into spring distros. This is 
good because 22.04 is an Ubuntu LTS release and I think they'd rather 
bump crocus versions to fix bugs than backport on top of i965. But 
that's really fort Ubuntu to decide. In any case, we won't see 
broad-spread usage and the flood of bug reports until next spring so 
we may want to wait until then to stay deleting code.


If we wanted to accelerate things, one option, once we're ready, would 
be to ask the person who manages the oibaf PPA to switch to crocus 
early. That may get some early adopters on board.


Thoughts?


I though the idea was to put everything in a classic branch and let 
distros run "classic" hardware from that. What happens after 3 releases 
does i965 still go to the classic branch with the other classic drivers? 
If so is it really worth waiting just because Ubuntu might have to 
back-port a bug fix?






--Jason

On April 9, 2021 22:09:14 Dylan Baker  wrote:


Quoting Dylan Baker (2021-03-22 15:15:30)

Hi list,

We've talked about it a number of times, but I think it's time time to
discuss splitting the classic drivers off of the main development branch
again, although this time I have a concrete plan for how this would
work.

First, why? Basically, all of the classic drivers are in maintanence
mode (even i965). Second, many of them rely on code that no one works
on, and very few people still understand. There is no CI for most of
them, and the Intel CI is not integrated with gitlab, so it's easy to
unintentionally break them, and this breakage usually isn't noticed
until just before or just after a release. 21.0 was held up (in small
part, also me just getting behind) because of such breakages.

I konw there is some interest in getting i915g in good enough shape that
it could replace i915c, at least for the common case. I also am aware
that Dave, Ilia, and Eric (with some pointers from Ken) have been
working on a gallium driver to replace i965. Neither of those things are
ready yet, but I've taken them into account.

Here's the plan:

1) 21.1 release happens
2) we remove classic from master
3) 21.1 reaches EOL because of 21.2
4) we fork the 21.1 branch into a "classic-lts"¹ branch
5) we disable all vulkan and gallium drivers in said branch, at least at
the Meson level
6) We change the name and precidence of the glvnd loader file
7) apply any build fixups (turn of intel generators for versions >= 7.5,
for example
8) maintain that branch with build and critical bug fixes only

This gives ditros and end users two options.
1) then can build *only* the legacy branch in the a normal Mesa provides
libGL interfaces fashion
2) They can use glvnd and install current mesa and the legacy branch in
parallel

Because of glvnd, we can control which driver will get loaded first, and
thus if we decide i915g or the i965 replacement is ready and turn it on
by default it will be loaded by default. An end user who doesn't like
this can add a new glvnd loader file that makes the classic drivers
higher precident and continue to use them.

Why fork from 21.1 instead of master?

First, it allows us to delete classic immediately, which will allow
refactoring to happen earlier in the cycle, and for any fallout to be
caught and hopefully fixed before the release. Second, it means that
when a user is switched from 21.1 to the new classic-lts branch, there
will be no regressions, and no one has to spend time figuring out what
broke and fixing the lts branch.

When you say "build and critical bug fixes", what do you mean?

I mean update Meson if we rely on something that in the future is
deprecated and removed, and would prevent building the branch or an
relying on some compiler behavior that changes, gaping exploitable
security holes, that kind of thing.

footnotes
¹Or whatever color you like your bikeshed


Here is a merge request to remove classic:
https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/10153

Dylan



___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org

Re: [Mesa-dev] [RFC] Concrete proposal to split classic

[Timothy Arceri]

On 3/23/21 7:26 PM, Ian Romanick wrote:


I would like to wait a couple more releases to do this.  I have a couple
things that I've been gradually working on for some of the non-i965
classic drivers that I'd like to land before they're put out to pasture.
  I talked to ajax about this a few weeks ago, and he was amenable at the
time.


Is there any reason these could not just land in the forked classic 
branch? The consensus seems to be we would still be making the odd 
release of these forked drivers, unlike how the DRI1 drivers were put 
out to pasture. Ken already outlined a desire to still be able to add 
occasional features to i965.


There is real momentum around cleaning up and optimising core Mesa at 
the moment, I'd really hate to see that slow down just because of a self 
imposed rule on what changes can land in a forked classic branch. I 
can't see there being to many MRs and releases required whether we 
applied a strict "build and critical bug fixes only" policy or just let 
any reasonable MR be accepted. I mean its not like there are MR flowing 
in for classic drivers now.


Just to be clear I'm +1 for forking 21.1.


On 3/22/21 3:15 PM, Dylan Baker wrote:

Hi list,

We've talked about it a number of times, but I think it's time time to
discuss splitting the classic drivers off of the main development branch
again, although this time I have a concrete plan for how this would
work.

First, why? Basically, all of the classic drivers are in maintanence
mode (even i965). Second, many of them rely on code that no one works
on, and very few people still understand. There is no CI for most of
them, and the Intel CI is not integrated with gitlab, so it's easy to
unintentionally break them, and this breakage usually isn't noticed
until just before or just after a release. 21.0 was held up (in small
part, also me just getting behind) because of such breakages.

I konw there is some interest in getting i915g in good enough shape that
it could replace i915c, at least for the common case. I also am aware
that Dave, Ilia, and Eric (with some pointers from Ken) have been
working on a gallium driver to replace i965. Neither of those things are
ready yet, but I've taken them into account.

Here's the plan:

1) 21.1 release happens
2) we remove classic from master
3) 21.1 reaches EOL because of 21.2
4) we fork the 21.1 branch into a "classic-lts"¹ branch
5) we disable all vulkan and gallium drivers in said branch, at least at
the Meson level
6) We change the name and precidence of the glvnd loader file
7) apply any build fixups (turn of intel generators for versions >= 7.5,
for example
8) maintain that branch with build and critical bug fixes only

This gives ditros and end users two options.
1) then can build *only* the legacy branch in the a normal Mesa provides
libGL interfaces fashion
2) They can use glvnd and install current mesa and the legacy branch in
parallel

Because of glvnd, we can control which driver will get loaded first, and
thus if we decide i915g or the i965 replacement is ready and turn it on
by default it will be loaded by default. An end user who doesn't like
this can add a new glvnd loader file that makes the classic drivers
higher precident and continue to use them.

Why fork from 21.1 instead of master?

First, it allows us to delete classic immediately, which will allow
refactoring to happen earlier in the cycle, and for any fallout to be
caught and hopefully fixed before the release. Second, it means that
when a user is switched from 21.1 to the new classic-lts branch, there
will be no regressions, and no one has to spend time figuring out what
broke and fixing the lts branch.

When you say "build and critical bug fixes", what do you mean?

I mean update Meson if we rely on something that in the future is
deprecated and removed, and would prevent building the branch or an
relying on some compiler behavior that changes, gaping exploitable
security holes, that kind of thing.

footnotes
¹Or whatever color you like your bikeshed


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] Small problem with my Mesa install (via Re: [ANNOUNCE] mesa 20.2.2)

[Timothy Arceri]

Looks like it might have been this bug 
https://gitlab.freedesktop.org/mesa/mesa/-/issues/3990 which should be 
fixed by 
https://gitlab.freedesktop.org/mesa/mesa/-/commit/58e43594fc457eaaf1b1e01e48948959a82080bc


On 2/11/21 10:40 PM, Ed B wrote:

Dear Dylan / Mesa Developer(s),

I have just managed to find this contact email for someone who is something to 
do with Mesa development / repair :-)

I have a problem with my computer crashing - probably due to Mesa updating from 
version 20.0.8 to 20.2.6, please see my enquiry to the KDE Forum 
https://forum.kde.org/viewtopic.php?f=63=169662  For further details.

I would be very grateful if you would you point me to the right person / Forum 
for dealing with this issue please (you will notice I am not a software person) 
?
And you are welcome to pass on this email.

For the person who can tell me, I would like to know -

Is this a "bug" which will be fixed ?
Is this old hardware which was removed from programming, and might be 
re-instated in the programming ?
Am I at a "dead-end" ?  and will have to forgo further Updates permanently - I 
do hope not :-/

I didn't mention on the Forum that I was having memory "segmentation" faults.   I have 
looked those up and they seem to be very and awkward to deal with - so I am hoping it is 
"just" a software bug which is fixable.

Currently I am using Kubuntu 20-10, I have moved from K-20-04, and tried Ubuntu 
20-04 on the way, to no avail.

Many thanks,  Regards,  Ed G8BQR
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] remaining 20.2 patches

[Timothy Arceri]

On 2/12/20 7:05 am, Dylan Baker wrote:


9659384744 ac/nir: fix a typo in ac_are_tessfactors_def_in_all_invocs


I'm surprised this doesn't apply cleanly as I don't believe this code 
has been changed in quite a while. However it is only a single line 
change so should be pretty easy to resolve any merge issues?

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] Mesa 20.2.x and GL_RG8_SNORM/GL_NONE

[Timothy Arceri]

So I found your thread [1]

Forcing no error is generally not recommended or a good idea. The game 
seems to work fine for me.


[1] 
https://forum.warthunder.com/index.php?/topic/500816-unsupported-formattype-gl_rg8_snormgl_none/


On 16/10/20 11:11 am, Daniel Mota Leite wrote:

Hi

Since i updated to mesa 20.2.0 and then to 20.2.1, i'm unable to
load war thunder game, it now just returns:

Unsupported format/type: GL_RG8_SNORM/GL_NONE

In the game forum another user complained about the same issue.
We both use AMD cards, RX480 and RX580 with the same mesa version.

Can someone translate what that error mean? may some mesa bug
affecting this cards slipped in to 20.2.x?

I'm on kernel 5.8.13 in slackware and the other user 5.7.10 in
gentoo.

Thanks in advance for the help
higuita


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] radv: Handle failing to create .cache dir. BISECTED - This one is broken

[Timothy Arceri]

This should fix it:

https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/5249

On 28/5/20 1:16 pm, Dieter Nützel wrote:

Hello Bas,

here is something lost/broken.
Part-of: 

If you have NO
~/.cache/radv_builtin_shaders64

it wouldn't be (re)created any longer.

Reverting #cd61f5234d2 solved it.

Thanks,
Dieter
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] Requiring a full author name when contributing to mesa?

[Timothy Arceri]

On 13/12/19 10:15 am, Alex Deucher wrote:

What does the name matter?  The name is the least of your worries.
What if their patch uses a patented algorithm?  Does anyone check for
that?  The whole Signed-off-by thing just just hazing for newbs.
Someone took the time to write and submit a patch.  We trust they did
the right thing and didn't do anything illegal. It's on the reviewers
to determine if the patch is reasonable and should be applied.  The
name is just window dressing.


Yes exactly the name is just window dressing, where a window dressing is 
"designed to create a favourable impression". In this case to make our 
project look competently and professionally run at a glance. I've been 
lucky enough to be employed to work on this project for around 5 years 
now, but don't expect this to last forever. Eventually I'd like to be 
able to point out to future employers the work I've been doing for all 
this time. Personally I'd like the window dressing to look nice when 
this time comes.


If other developers don't care which was my original question then I'll 
stop wasting my time requesting people not use such author names.





Alex

On Thu, Dec 12, 2019 at 5:42 PM Timothy Arceri  wrote:




On 13/12/19 1:54 am, Daniel Stone wrote:

On Wed, 11 Dec 2019 at 22:35, Timothy Arceri  wrote:

So it seems lately we have been increasingly merging patches with made
up names, or single names etc [1]. The latest submitted patch has the
name Icecream95. This seems wrong to me from a point of keeping up the
integrity of the project. I'm not a legal expert but it doesn't seem
ideal to be amassing commits with these type of author tags from that
point of view either.

Is it just me or do others agree we should at least require a proper
name on the commits (as fake as that may be also)? Seems like a low bar
to me.


What benefit does it bring?

Icecream95 could just resubmit as 'John Johnson'; would we just take
that as face value that that was their 'real name' and accept the
contribution?

I know someone in Australia who changed their name via deed poll to
Stormy Wrathcauser (changed slightly to protect their privacy, but
very close). Would we accept their contribution if they posted, or
would we have to stop and take measures to verify that that was their
real legal name?

What about Chinese contributors, who as noted in thread tend to use
made-up non-legal pseudonyms anyway?

Unless we're actually trying to bring up and enforce a web of trust, I
don't think there's any point in requiring that the submitter's name
conforms to some notion of idealised naming - it's just window
dressing. I also don't see any point in trying to enforce a web of
trust. Debian's method of doing this involves a hundred people
standing around in a room looking at drivers' licenses from countries
they might not have even heard of before to verify identity. But I'm
certainly not an expert at identifying whether or not a Bolivian
drivers' license which is put in front of my face is forged or not,
and suspect no-one on this list is.

If someone is determined to compromise the legal integrity of Mesa's
codebase, requiring that they register as Juan Molinos or any other
name which seems like it could be 'legitimate' is not really any
barrier to entry.



Hi Daniel,

I've already given my personal thoughts on all these questions in the
various threads, ultimately I was just asking if we should use a little
common sense here. If people don't want to apply this extremely low bar,
then so be it. Let the contributions from atom symbols and inanimate
objects flow in.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] Requiring a full author name when contributing to mesa?

[Timothy Arceri]

On 13/12/19 1:54 am, Daniel Stone wrote:

On Wed, 11 Dec 2019 at 22:35, Timothy Arceri  wrote:

So it seems lately we have been increasingly merging patches with made
up names, or single names etc [1]. The latest submitted patch has the
name Icecream95. This seems wrong to me from a point of keeping up the
integrity of the project. I'm not a legal expert but it doesn't seem
ideal to be amassing commits with these type of author tags from that
point of view either.

Is it just me or do others agree we should at least require a proper
name on the commits (as fake as that may be also)? Seems like a low bar
to me.


What benefit does it bring?

Icecream95 could just resubmit as 'John Johnson'; would we just take
that as face value that that was their 'real name' and accept the
contribution?

I know someone in Australia who changed their name via deed poll to
Stormy Wrathcauser (changed slightly to protect their privacy, but
very close). Would we accept their contribution if they posted, or
would we have to stop and take measures to verify that that was their
real legal name?

What about Chinese contributors, who as noted in thread tend to use
made-up non-legal pseudonyms anyway?

Unless we're actually trying to bring up and enforce a web of trust, I
don't think there's any point in requiring that the submitter's name
conforms to some notion of idealised naming - it's just window
dressing. I also don't see any point in trying to enforce a web of
trust. Debian's method of doing this involves a hundred people
standing around in a room looking at drivers' licenses from countries
they might not have even heard of before to verify identity. But I'm
certainly not an expert at identifying whether or not a Bolivian
drivers' license which is put in front of my face is forged or not,
and suspect no-one on this list is.

If someone is determined to compromise the legal integrity of Mesa's
codebase, requiring that they register as Juan Molinos or any other
name which seems like it could be 'legitimate' is not really any
barrier to entry.



Hi Daniel,

I've already given my personal thoughts on all these questions in the 
various threads, ultimately I was just asking if we should use a little 
common sense here. If people don't want to apply this extremely low bar, 
then so be it. Let the contributions from atom symbols and inanimate 
objects flow in.

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] Requiring a full author name when contributing to mesa?

[Timothy Arceri]

On 12/12/19 10:38 am, Eric Engestrom wrote:

On 2019-12-11 at 23:09, Eric Anholt  wrote:

On Wed, Dec 11, 2019 at 2:35 PM Timothy Arceri  wrote:


Hi,

So it seems lately we have been increasingly merging patches with made
up names, or single names etc [1]. The latest submitted patch has the
name Icecream95. This seems wrong to me from a point of keeping up the
integrity of the project. I'm not a legal expert but it doesn't seem
ideal to be amassing commits with these type of author tags from that
point of view either.

Is it just me or do others agree we should at least require a proper
name on the commits (as fake as that may be also)? Seems like a low bar
to me.


I'm of the opinion that in fact all names are made up,


Whole heartedly agreed.

Remember that many different cultures exist, and they have different customs
around names. As an example, a teacher of mine had a single name, but the school
required two separate "first name" and "last name" fields so he wrote his name 
twice,
which appeared on every form we got from the school, yet everyone knew he didn't
have what we called a "last name"/"family name".
Another example is people from Asia who often assume a made up Western-sounding
pseudonym to use when communicating with Western people, and those often don't
look like real names to us.

What looks like a real name to you?
How would you even start to define such a rule?


As per my reply to Eric Anholt I'm most concerned about the look of the 
project. IMO contributions with names like Icecream95 or an atom symbol 
just look unprofessional, opensource gets a hard enough time about its 
professionalism as it is without encouraging this. A little common sense 
can go a long way here.





and we don't
want to be getting into the business of requiring legal names for
committing.  If legal names were what you were getting at: have you
checked the legal names of your fellow contributors match what they're
contributing under?

I don't know what legal risk you might be thinking of, that seems like
spreading fear for no reason to me.

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] Requiring a full author name when contributing to mesa?

[Timothy Arceri]

On 12/12/19 10:09 am, Eric Anholt wrote:

On Wed, Dec 11, 2019 at 2:35 PM Timothy Arceri  wrote:


Hi,

So it seems lately we have been increasingly merging patches with made
up names, or single names etc [1]. The latest submitted patch has the
name Icecream95. This seems wrong to me from a point of keeping up the
integrity of the project. I'm not a legal expert but it doesn't seem
ideal to be amassing commits with these type of author tags from that
point of view either.

Is it just me or do others agree we should at least require a proper
name on the commits (as fake as that may be also)? Seems like a low bar
to me.


I'm of the opinion that in fact all names are made up, and we don't
want to be getting into the business of requiring legal names for
committing.  If legal names were what you were getting at: have you
checked the legal names of your fellow contributors match what they're
contributing under?


My question was: "do others agree we should at least require a proper 
name on the commits (as fake as that may be also)?"




I don't know what legal risk you might be thinking of, that seems like
spreading fear for no reason to me.


If the implications aren't obvious I'm not going to bother arguing 
further, as I'm not a legal expert. I'm more concerned about the look of 
the project. IMO contributions with names like Icecream95 just look 
unprofessional.

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] Requiring a full author name when contributing to mesa?

[Timothy Arceri]

Hi,

So it seems lately we have been increasingly merging patches with made 
up names, or single names etc [1]. The latest submitted patch has the 
name Icecream95. This seems wrong to me from a point of keeping up the 
integrity of the project. I'm not a legal expert but it doesn't seem 
ideal to be amassing commits with these type of author tags from that 
point of view either.


Is it just me or do others agree we should at least require a proper 
name on the commits (as fake as that may be also)? Seems like a low bar 
to me.



[1] https://gitlab.freedesktop.org/mesa/mesa/merge_requests/3050#note_361924
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] Is it time to stop using the mailing list for patch review?

[Timothy Arceri]

On 11/12/19 8:54 am, Alex Deucher wrote:

On Tue, Dec 10, 2019 at 1:13 PM Dylan Baker  wrote:


Do we have those and does anyone notice? I personally rarely look at the list
now unless I'm CC'd on something. That seems really bad for drive by
contributors.

But frankly I wouldn't submit to a mailing list as a drive by, it's more work to
get subscribed to mail man (so my patch goes through), set up get-send-email,
and send the patches, then unsubscribe when I'm done than it would be to sign up
for gitlab.fdo using one of the "sign-in-with" options. If you're not subscribed
you go into limbo until a list maintainer approves your patch, and I think any
follow ups. That seems even worse as most people probably aren't aware of that
behavior. Maybe I'm the only one who feels that way though.


I've still seen a few.


The only ones I've seen recently are from some new ? AMD devs and a few 
patches from Jonathan Gray who is a long time contributor.


Having merge request run though basic CI before merging is enough reason 
to no longer suggest people use the mailing list for patch submission. 
I've created a MR to update the docs.


https://gitlab.freedesktop.org/mesa/mesa/merge_requests/3045


 Most drive by contributors generally don't
subscribe to the list.  Since most lists are moderated, the mail
usually makes it through whether they are subscribed or not.  That how
most projects work.  I'm not subscribed to every kernel subsystem
list, but my messages usually make it through.  If we are proposing to
do away with the mailing list, what is the plan for non-patch
discussions?  Opening issues in gitlab?


I don't think this was the suggestion. For non-patch review discussions 
the list still makes sense. The great thing now is that without patches 
being sent to the list its easier to actually notice these discussions.




Alex



Dylan

Quoting Alex Deucher (2019-12-10 07:30:43)

How do we deal with drive by fixes?  E.g., some random user submits a
fix but doesn't want to create a gitlab account just to submit a fix?
Whoever reviews the patch should submit an MR?

Alex

On Mon, Dec 9, 2019 at 6:07 PM Dylan Baker  wrote:


Hi everyone,

I think its time we discussed whether we're going to continue to do patch review
on the mailing list, or if it it should all go through gitlab. I think we should
stop using the mailing list, here are some reasons:

1) Most development is happening on gitlab at this point, patches on the mailing
list are often overlooked
2) The mailing list bypasses CI which potentially breaks the build
3) Probably more reasons I'm forgetting.

Please discuss,
Dylan
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] Is it time to stop using the mailing list for patch review?

[Timothy Arceri]

On 11/12/19 5:58 am, Zebediah Figura wrote:

On 12/10/19 12:21 PM, Matt Turner wrote:

On Mon, Dec 9, 2019 at 6:07 PM Dylan Baker  wrote:


Hi everyone,

I think its time we discussed whether we're going to continue to do 
patch review
on the mailing list, or if it it should all go through gitlab. I 
think we should

stop using the mailing list, here are some reasons:

1) Most development is happening on gitlab at this point, patches on 
the mailing

    list are often overlooked
2) The mailing list bypasses CI which potentially breaks the build
3) Probably more reasons I'm forgetting.


I think effectively we're already there.

What concrete change would you propose?


Removing mention of the mailing list from documentation would be nice. 
Also, currently the README implies that the mailing list is not only 
acceptable but preferred: "Note that Mesa uses email mailing-lists for 
patches submission, review and discussions."


I've sent a merge request:

https://gitlab.freedesktop.org/mesa/mesa/merge_requests/3045



Relatedly, is GitLab preferred also for piglit? The HACKING file there 
also specifically prescribes using the mailing list, but I've noticed 
that patches submitted to the mailing list by myself and another new 
contributor have gone unreviewed for several weeks.


Yes we should update the piglit docs also.




___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev



___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] Remove classic drivers or fork src/mesa for gallium?

[Timothy Arceri]

On 4/12/19 3:45 pm, Jason Ekstrand wrote:
On Tue, Dec 3, 2019 at 8:19 PM Dave Airlie > wrote:


On Wed, 4 Dec 2019 at 10:39, Marek Olšák mailto:mar...@gmail.com>> wrote:
 >
 > Hi,
 >
 > Here are 2 proposals to simplify and better optimize the
GL->Gallium translation.
 >
 > 1) Move classic drivers to a fork of Mesa, and remove them from
master. Classic drivers won't share any code with master. glvnd will
load them, but glvnd is not ready for this yet.


I'm usually the first person to tell people to stop mucking about with 
old hardware and I fear even I think this would be a bit premature.  
We've not turned iris on by default yet and I'd really like it to bake 
as the default distro driver for a while (maybe a year) before we put 
i965 on life-support.  If we were having this discussion in another 
year's time, I might be in favor of this but, as it currently stands, 
I'm not sure this is a good idea.


 > 2) Keep classic drivers. Fork src/mesa for Gallium. I think only
mesa/main, mesa/vbo, mesa/program, and drivers/dri/common need to be
forked and mesa/state_tracker moved. src/gallium/state-trackers/gl/
can be the target location.


I really don't like this option.  For one thing, this means two copies 
of code that will get out-of-sync.  If we had the classic drivers in 
their own branch, we could at least cherry-pick bugfixes back to the 
classic branch.  However, if we have two copies of everything, our 
version control tool can't help us.


The bigger problem is headers.  Currently, the GLSL compiler includes 
mtypes.h so we would need to somehow separate the GLSL compiler out from 
src/mesa/main because we'll have two mtypes.h files.  Unfortunately, 
GLSL isn't the only thing that includes stuff from src/mesa/main.  Our 
header situation has been a disaster for years and I'm afraid it hasn't 
gotten any better with the addition of src/util.  Most people who move 
stuff don't actually do due diligence to keep the includes clean and the 
result is that I'm pretty sure even the Vulkan drivers are including 
src/mesa/main/imports.h.  If we have two copies of src/mesa/main and the 
headers begin to diverge, it's going to be a total disaster.  I would 
love to see someone clean this all up properly and I think Mesa would be 
better for it.  However, it is a LOT of work.


If we did the above cleaning, would I be ok with this approach?  
Possibly.  However, I suspect it'd end up being the worst of both 
worlds.  We still have maintenance of effectively two branches and 
bugfixes have to be ported.  At the same time, we'd still have core 
changes to things like the GLSL compiler breaking old drivers so we 
wouldn't lighten any of the maintenance burden.


 > Option 2 is more acceptable to people who want to keep classic
drivers in the tree and it can be done right now.

These both seem pretty horrible to me right now. Like i965 still
supports a lot of hardware that exists right now even if we move to
iris.


I'm less concerned about the support burden.  When it comes to bugfixes, 
I feel like most of the bugs on gen7 and earlier hardware at this point 
are due to churn in the core and if it lives in a maintenance branch, 
we'll stop breaking it.  The biggest thing we'd loose would be 
additional features that we might get thanks to core changes.  The major 
ones that come to mind are:


  - GL_ARB_gl_spirv (It could be enabled on gen7 in theory but we 
haven't due to a lack of testing) >   - GL_EXT_direct_state_access (still underway last I knew)


Is already done and enabled for compat profile.


  - GL_EXT_gpu_shader4


Core mesa parts are done for this also.


  - Compat context support


radeonsi, nouveau and iris all fully enable this. At this point its just 
a driver/classic issue to add support. So as with the two other features 
above options 1 or 2 will not change the fate of this support.




All four of those are more-or-less software-only features that older 
intel drivers could pick up as the core advances.  Compat support likely 
requires a bit of driver work but it should be doable.


The real question is, "How much do we care?"  I can't say I like the 
idea of leaving Gen4-7 out in the cold.  Again, in a year's time, we 
might have most of the above implemented and then it'd be less of a concern.


As per above its only GL_ARB_gl_spirv that would maybe be impacted by 
any fork, but that doesn't change between now and a year so delaying a 
decision for a year based on this is doesn't seem necessary.




I sorta feel there should be a
3) make life harder for classic drivers and optimise things more for
gallium add more dd.h entrypoints, force the classic drivers to jump
through hoops to degallium.


I'd be interested to see what that would look like.  I'm not actually 
convinced that it would work if I'm honest.  Part of gluing them 
together in my mind is deleting dd.h 

[Mesa-dev] [PATCH] mesa: remove super old TODOs from shaderapi.c

[Timothy Arceri]

---
 src/mesa/main/shaderapi.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
index 1351f7fd06f..607d30e2bd4 100644
--- a/src/mesa/main/shaderapi.c
+++ b/src/mesa/main/shaderapi.c
@@ -29,11 +29,6 @@
  *
  * Implementation of GLSL-related API functions.
  * The glUniform* functions are in uniforms.c
- *
- *
- * XXX things to do:
- * 1. Check that the right error code is generated for all _mesa_error() calls.
- * 2. Insert FLUSH_VERTICES calls in various places
  */
 
 
-- 
2.21.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] drirc: Add workaround for Divinity: Original Sin Enhanced Edition

[Timothy Arceri]

I ended up creating a merge request for this and another patch:

https://gitlab.freedesktop.org/mesa/mesa/merge_requests/1550

On 2/8/19 10:09 am, Timothy Arceri wrote:

This adds an additional work around for the game to fix the blocky
shaders as reported in bug 105282

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105282
---
  src/util/00-mesa-defaults.conf | 1 +
  1 file changed, 1 insertion(+)

diff --git a/src/util/00-mesa-defaults.conf b/src/util/00-mesa-defaults.conf
index ae29d8837d5..34a7993a314 100644
--- a/src/util/00-mesa-defaults.conf
+++ b/src/util/00-mesa-defaults.conf
@@ -146,6 +146,7 @@ TODO: document the other workarounds.
  
  

  
+
  
  
  



___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] egl: fix OpenGL 3.1 context creation

[Timothy Arceri]

From the EGL_KHR_create_context spec:

   "* If OpenGL 3.1 is requested, the context returned may implement
   any of the following versions:

 * Version 3.1. The GL_ARB_compatibility extension may or may
   not be implemented, as determined by the implementation.
 * The core profile of version 3.2 or greater."

Fixes CTS tests:

dEQP-EGL.functional.create_context_ext.gl_31.rgb888_depth_stencil
dEQP-EGL.functional.create_context_ext.robust_gl_31.rgb888_depth_stencil
dEQP-EGL.functional.create_context_ext.gl_31.rgb888_depth_no_stencil
dEQP-EGL.functional.create_context_ext.robust_gl_31.rgb888_depth_no_stencil
dEQP-EGL.functional.create_context_ext.gl_31.rgba_depth_no_stencil
dEQP-EGL.functional.create_context_ext.gl_31.rgb888_no_depth_no_stencil

dEQP-EGL.functional.create_context_ext.robust_gl_31.rgba_depth_no_stencil

dEQP-EGL.functional.create_context_ext.robust_gl_31.rgb888_no_depth_no_stencil
dEQP-EGL.functional.create_context_ext.gl_31.rgba_no_depth_no_stencil

dEQP-EGL.functional.create_context_ext.robust_gl_31.rgba_no_depth_no_stencil
dEQP-EGL.functional.create_context_ext.gl_31.rgba_depth_stencil
dEQP-EGL.functional.create_context_ext.robust_gl_31.rgba_depth_stencil
---
 src/egl/drivers/dri2/egl_dri2.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index c712c106b06..918d61a1e9b 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -1245,6 +1245,9 @@ dri2_create_context(_EGLDriver *drv, _EGLDisplay *disp, 
_EGLConfig *conf,
&& dri2_ctx->base.ClientMinorVersion >= 2))
   && dri2_ctx->base.Profile == EGL_CONTEXT_OPENGL_CORE_PROFILE_BIT_KHR)
  api = __DRI_API_OPENGL_CORE;
+  else if (dri2_ctx->base.ClientMajorVersion == 3 &&
+   dri2_ctx->base.ClientMinorVersion == 1)
+ api = __DRI_API_OPENGL_CORE;
   else
  api = __DRI_API_OPENGL;
   break;
-- 
2.21.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] drirc: Add workaround for Divinity: Original Sin Enhanced Edition

[Timothy Arceri]

This adds an additional work around for the game to fix the blocky
shaders as reported in bug 105282

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105282
---
 src/util/00-mesa-defaults.conf | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/util/00-mesa-defaults.conf b/src/util/00-mesa-defaults.conf
index ae29d8837d5..34a7993a314 100644
--- a/src/util/00-mesa-defaults.conf
+++ b/src/util/00-mesa-defaults.conf
@@ -146,6 +146,7 @@ TODO: document the other workarounds.
 
 
 
+
 
 
 
-- 
2.21.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] mesa: save/restore SSO flag when using ARB_get_program_binary

[Timothy Arceri]

Ping! The spec bug was updated and they have agreed to update the spec 
to define this behavior.


I've also sent a piglit test for this:

https://patchwork.freedesktop.org/patch/317112/

On 1/7/19 12:25 pm, Timothy Arceri wrote:

Without this the restored program will fail the pipeline validation
checks when we attempt to use an SSO program.

Fixes: c20fd744fef1 ("mesa: Add Mesa ARB_get_program_binary helper functions")

Cc: Jordan Justen 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111010
---
  src/mesa/main/program_binary.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/src/mesa/main/program_binary.c b/src/mesa/main/program_binary.c
index 7390fef5887..39537cfccce 100644
--- a/src/mesa/main/program_binary.c
+++ b/src/mesa/main/program_binary.c
@@ -178,6 +178,8 @@ write_program_payload(struct gl_context *ctx, struct blob 
*blob,
shader->Program);
 }
  
+   blob_write_uint32(blob, sh_prog->SeparateShader);

+
 serialize_glsl_program(blob, ctx, sh_prog);
  
 for (unsigned stage = 0; stage < MESA_SHADER_STAGES; stage++) {

@@ -195,6 +197,8 @@ static bool
  read_program_payload(struct gl_context *ctx, struct blob_reader *blob,
   GLenum binary_format, struct gl_shader_program *sh_prog)
  {
+   sh_prog->SeparateShader = blob_read_uint32(blob);
+
 if (!deserialize_glsl_program(blob, ctx, sh_prog))
return false;
  


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] radv: fix memory leak when restoring from cache

[Timothy Arceri]

Fixes: 726a31df705b ("radv: Add the concept of radv shader binaries.")
---
 src/amd/vulkan/radv_pipeline_cache.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/amd/vulkan/radv_pipeline_cache.c 
b/src/amd/vulkan/radv_pipeline_cache.c
index 2b3fda6eb8e..b773de30c32 100644
--- a/src/amd/vulkan/radv_pipeline_cache.c
+++ b/src/amd/vulkan/radv_pipeline_cache.c
@@ -309,6 +309,7 @@ radv_create_shader_variants_from_pipeline_cache(struct 
radv_device *device,
p += entry->binary_sizes[i];
 
entry->variants[i] = radv_shader_variant_create(device, 
binary);
+   free(binary);
} else if (entry->binary_sizes[i]) {
p += entry->binary_sizes[i];
}
-- 
2.21.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] nir/lower_io_to_temporaries: Fix hash table leak

[Timothy Arceri]

Reviewed-by: Timothy Arceri 

On 9/7/19 2:20 am, Connor Abbott wrote:

Fixes: c45f5db527252384395e55fb1149b673ec7b5fa8 ("nir/lower_io_to_temporaries: 
Handle interpolation intrinsics")
---
Whoops...

  src/compiler/nir/nir_lower_io_to_temporaries.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/src/compiler/nir/nir_lower_io_to_temporaries.c 
b/src/compiler/nir/nir_lower_io_to_temporaries.c
index c865c7de10c..f92489b9d51 100644
--- a/src/compiler/nir/nir_lower_io_to_temporaries.c
+++ b/src/compiler/nir/nir_lower_io_to_temporaries.c
@@ -364,4 +364,6 @@ nir_lower_io_to_temporaries(nir_shader *shader, 
nir_function_impl *entrypoint,
 exec_list_append(>globals, _outputs);
  
 nir_fixup_deref_modes(shader);

+
+   _mesa_hash_table_destroy(state.input_map, NULL);
  }


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] gallium: remove boolean from state tracker APIs

[Timothy Arceri]

Looks good to me.

Reviewed-by: Timothy Arceri 

On 5/7/19 5:36 am, Ilia Mirkin wrote:

Signed-off-by: Ilia Mirkin 
---

It's actually a bit tricky to compile all the bits involved, so some of
this is untested. However if it so happens that I missed a spot, it
should be trivial to fix.

  src/gallium/include/state_tracker/graw.h  |  8 +-
  src/gallium/include/state_tracker/st_api.h| 73 +--
  src/gallium/include/state_tracker/sw_winsys.h |  5 +-
  src/gallium/state_trackers/dri/dri_drawable.c | 34 -
  src/gallium/state_trackers/dri/dri_drawable.h |  6 +-
  src/gallium/state_trackers/dri/dri_screen.c   |  8 +-
  src/gallium/state_trackers/glx/xlib/xm_st.c   | 24 +++---
  src/gallium/state_trackers/glx/xlib/xm_st.h   |  2 +-
  src/gallium/state_trackers/hgl/hgl.c  | 14 ++--
  src/gallium/state_trackers/osmesa/osmesa.c|  8 +-
  src/gallium/state_trackers/wgl/stw_st.c   | 14 ++--
  src/gallium/state_trackers/wgl/stw_st.h   |  2 +-
  src/gallium/targets/graw-null/graw_util.c | 14 ++--
  src/gallium/winsys/sw/dri/dri_sw_winsys.c | 12 +--
  src/gallium/winsys/sw/gdi/gdi_sw_winsys.c | 10 +--
  src/gallium/winsys/sw/hgl/hgl_sw_winsys.c |  8 +-
  .../winsys/sw/kms-dri/kms_dri_sw_winsys.c | 12 +--
  src/gallium/winsys/sw/null/null_sw_winsys.c   |  8 +-
  .../winsys/sw/wrapper/wrapper_sw_winsys.c | 10 +--
  src/gallium/winsys/sw/xlib/xlib_sw_winsys.c   | 14 ++--
  src/mesa/state_tracker/st_manager.c   | 50 ++---
  src/mesa/state_tracker/st_manager.h   |  2 +-
  22 files changed, 168 insertions(+), 170 deletions(-)

diff --git a/src/gallium/include/state_tracker/graw.h 
b/src/gallium/include/state_tracker/graw.h
index 78ddf0a87f7..af81cc8871b 100644
--- a/src/gallium/include/state_tracker/graw.h
+++ b/src/gallium/include/state_tracker/graw.h
@@ -79,7 +79,7 @@ PUBLIC void *graw_parse_fragment_shader( struct pipe_context 
*pipe,
   * If an option has been successfully parsed, argi is updated
   * to point just after the option and return TRUE.
   */
-PUBLIC boolean graw_parse_args(int *argi, int argc, char *argv[]);
+PUBLIC bool graw_parse_args(int *argi, int argc, char *argv[]);
  
  /* Saves surface contents to a file.

   *
@@ -89,8 +89,8 @@ PUBLIC boolean graw_parse_args(int *argi, int argc, char 
*argv[]);
   *
   * Returns TRUE if the surface has been saved.
   */
-PUBLIC boolean graw_save_surface_to_file(struct pipe_context *pipe,
- struct pipe_surface *surface,
- const char *filename);
+PUBLIC bool graw_save_surface_to_file(struct pipe_context *pipe,
+  struct pipe_surface *surface,
+  const char *filename);
  
  #endif

diff --git a/src/gallium/include/state_tracker/st_api.h 
b/src/gallium/include/state_tracker/st_api.h
index 2b63b8a3d2a..b2b81b6ebc4 100644
--- a/src/gallium/include/state_tracker/st_api.h
+++ b/src/gallium/include/state_tracker/st_api.h
@@ -27,7 +27,6 @@
  #ifndef _ST_API_H_
  #define _ST_API_H_
  
-#include "pipe/p_compiler.h"

  #include "pipe/p_format.h"
  
  /**

@@ -218,19 +217,19 @@ struct st_visual
   */
  struct st_config_options
  {
-   boolean disable_blend_func_extended;
-   boolean disable_glsl_line_continuations;
-   boolean force_glsl_extensions_warn;
+   bool disable_blend_func_extended;
+   bool disable_glsl_line_continuations;
+   bool force_glsl_extensions_warn;
 unsigned force_glsl_version;
-   boolean allow_glsl_extension_directive_midshader;
-   boolean allow_glsl_builtin_const_expression;
-   boolean allow_glsl_relaxed_es;
-   boolean allow_glsl_builtin_variable_redeclaration;
-   boolean allow_higher_compat_version;
-   boolean glsl_zero_init;
-   boolean force_glsl_abs_sqrt;
-   boolean allow_glsl_cross_stage_interpolation_mismatch;
-   boolean allow_glsl_layout_qualifier_on_function_parameters;
+   bool allow_glsl_extension_directive_midshader;
+   bool allow_glsl_builtin_const_expression;
+   bool allow_glsl_relaxed_es;
+   bool allow_glsl_builtin_variable_redeclaration;
+   bool allow_higher_compat_version;
+   bool glsl_zero_init;
+   bool force_glsl_abs_sqrt;
+   bool allow_glsl_cross_stage_interpolation_mismatch;
+   bool allow_glsl_layout_qualifier_on_function_parameters;
 unsigned char config_options_sha1[20];
  };
  
@@ -319,9 +318,9 @@ struct st_framebuffer_iface

  *
  * @att is one of the front buffer attachments.
  */
-   boolean (*flush_front)(struct st_context_iface *stctx,
-  struct st_framebuffer_iface *stfbi,
-  enum st_attachment_type statt);
+   bool (*flush_front)(struct st_context_iface *stctx,
+   struct st_framebuffer_iface *stfbi,
+   enum st_attachment_type statt);
  
 /**

  * The state tracker asks for the textures it needs.
@@ -340,13 +339,13 @@ struct 

Re: [Mesa-dev] [PATCH] mesa: save/restore SSO flag when using ARB_get_program_binary

[Timothy Arceri]

On 1/7/19 2:24 pm, Timothy Arceri wrote:
FYI since the spec doesn't specifically cover what to do in this case I 
have filed a spec bug suggesting it be updated to follow this behavior.


https://gitlab.khronos.org/opengl/GLSL/issues/46


Whoops filed that one against GLSL rather than the API here is the 
correct link:


https://gitlab.khronos.org/opengl/API/issues/114



On 1/7/19 12:25 pm, Timothy Arceri wrote:

Without this the restored program will fail the pipeline validation
checks when we attempt to use an SSO program.

Fixes: c20fd744fef1 ("mesa: Add Mesa ARB_get_program_binary helper 
functions")


Cc: Jordan Justen 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111010
---
  src/mesa/main/program_binary.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/src/mesa/main/program_binary.c 
b/src/mesa/main/program_binary.c

index 7390fef5887..39537cfccce 100644
--- a/src/mesa/main/program_binary.c
+++ b/src/mesa/main/program_binary.c
@@ -178,6 +178,8 @@ write_program_payload(struct gl_context *ctx, 
struct blob *blob,

    shader->Program);
 }
+   blob_write_uint32(blob, sh_prog->SeparateShader);
+
 serialize_glsl_program(blob, ctx, sh_prog);
 for (unsigned stage = 0; stage < MESA_SHADER_STAGES; stage++) {
@@ -195,6 +197,8 @@ static bool
  read_program_payload(struct gl_context *ctx, struct blob_reader *blob,
   GLenum binary_format, struct gl_shader_program 
*sh_prog)

  {
+   sh_prog->SeparateShader = blob_read_uint32(blob);
+
 if (!deserialize_glsl_program(blob, ctx, sh_prog))
    return false;


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] mesa: save/restore SSO flag when using ARB_get_program_binary

[Timothy Arceri]

FYI since the spec doesn't specifically cover what to do in this case I 
have filed a spec bug suggesting it be updated to follow this behavior.


https://gitlab.khronos.org/opengl/GLSL/issues/46

On 1/7/19 12:25 pm, Timothy Arceri wrote:

Without this the restored program will fail the pipeline validation
checks when we attempt to use an SSO program.

Fixes: c20fd744fef1 ("mesa: Add Mesa ARB_get_program_binary helper functions")

Cc: Jordan Justen 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111010
---
  src/mesa/main/program_binary.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/src/mesa/main/program_binary.c b/src/mesa/main/program_binary.c
index 7390fef5887..39537cfccce 100644
--- a/src/mesa/main/program_binary.c
+++ b/src/mesa/main/program_binary.c
@@ -178,6 +178,8 @@ write_program_payload(struct gl_context *ctx, struct blob 
*blob,
shader->Program);
 }
  
+   blob_write_uint32(blob, sh_prog->SeparateShader);

+
 serialize_glsl_program(blob, ctx, sh_prog);
  
 for (unsigned stage = 0; stage < MESA_SHADER_STAGES; stage++) {

@@ -195,6 +197,8 @@ static bool
  read_program_payload(struct gl_context *ctx, struct blob_reader *blob,
   GLenum binary_format, struct gl_shader_program *sh_prog)
  {
+   sh_prog->SeparateShader = blob_read_uint32(blob);
+
 if (!deserialize_glsl_program(blob, ctx, sh_prog))
return false;
  


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] mesa: save/restore SSO flag when using ARB_get_program_binary

[Timothy Arceri]

Without this the restored program will fail the pipeline validation
checks when we attempt to use an SSO program.

Fixes: c20fd744fef1 ("mesa: Add Mesa ARB_get_program_binary helper functions")

Cc: Jordan Justen 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111010
---
 src/mesa/main/program_binary.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/mesa/main/program_binary.c b/src/mesa/main/program_binary.c
index 7390fef5887..39537cfccce 100644
--- a/src/mesa/main/program_binary.c
+++ b/src/mesa/main/program_binary.c
@@ -178,6 +178,8 @@ write_program_payload(struct gl_context *ctx, struct blob 
*blob,
   shader->Program);
}
 
+   blob_write_uint32(blob, sh_prog->SeparateShader);
+
serialize_glsl_program(blob, ctx, sh_prog);
 
for (unsigned stage = 0; stage < MESA_SHADER_STAGES; stage++) {
@@ -195,6 +197,8 @@ static bool
 read_program_payload(struct gl_context *ctx, struct blob_reader *blob,
  GLenum binary_format, struct gl_shader_program *sh_prog)
 {
+   sh_prog->SeparateShader = blob_read_uint32(blob);
+
if (!deserialize_glsl_program(blob, ctx, sh_prog))
   return false;
 
-- 
2.21.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] st/glsl: fix silly regression finding gs/tes variants

[Timothy Arceri]

Fixes: d19fe5e67a39 ("st/glsl: support clamping color outputs in compat for 
gs/tes")
---
 src/mesa/state_tracker/st_program.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/mesa/state_tracker/st_program.c 
b/src/mesa/state_tracker/st_program.c
index 3c99721a8a7..faada1621ca 100644
--- a/src/mesa/state_tracker/st_program.c
+++ b/src/mesa/state_tracker/st_program.c
@@ -1641,7 +1641,7 @@ st_get_basic_variant(struct st_context *st,
 
/* Search for existing variant */
for (v = prog->variants; v; v = v->next) {
-  if (memcmp(>key, , sizeof(key)) == 0) {
+  if (memcmp(>key, key, sizeof(*key)) == 0) {
  break;
   }
}
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] nir/loop_analyze: used nir_alu_src to track loop limit

[Timothy Arceri]

On 20/6/19 5:57 am, Jason Ekstrand wrote:
On Wed, Jun 19, 2019 at 3:09 AM Timothy Arceri <mailto:tarc...@itsqueeze.com>> wrote:


This helps reduce the amount of abstraction in this pass and allows
us to retain more information about the src such as any swizzles.
Retaining the swizzle information is required for a bugfix in the
following patch.

Fixes: 6772a17acc8e ("nir: Add a loop analysis pass")
---
  src/compiler/nir/nir_loop_analyze.c | 37 +++--
  1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/src/compiler/nir/nir_loop_analyze.c
b/src/compiler/nir/nir_loop_analyze.c
index e85a404da1b..57d2d94cad2 100644
--- a/src/compiler/nir/nir_loop_analyze.c
+++ b/src/compiler/nir/nir_loop_analyze.c
@@ -543,25 +543,26 @@ guess_loop_limit(loop_info_state *state,
nir_const_value *limit_val,
  }

  static bool
-try_find_limit_of_alu(nir_loop_variable *limit, nir_const_value
*limit_val,
-                      nir_loop_terminator *terminator,
loop_info_state *state)
+try_find_limit_of_alu(nir_alu_src *limit, nir_const_value *limit_val,
+                      nir_loop_terminator *terminator)
  {
-   if(!is_var_alu(limit))
+   if(limit->src.ssa->parent_instr->type != nir_instr_type_alu)
        return false;

-   nir_alu_instr *limit_alu =
nir_instr_as_alu(limit->def->parent_instr);
+   nir_alu_instr *limit_alu =
nir_instr_as_alu(limit->src.ssa->parent_instr);

     if (limit_alu->op == nir_op_imin ||
         limit_alu->op == nir_op_fmin) {
-      limit = get_loop_var(limit_alu->src[0].src.ssa, state);
+      limit = _alu->src[0];

-      if (!is_var_constant(limit))
-         limit = get_loop_var(limit_alu->src[1].src.ssa, state);
+      if (limit->src.ssa->parent_instr->type !=
nir_instr_type_load_const)
+         limit = _alu->src[1];


This is still horribly broken w.r.t swizzles because we're not tracking 
the component as we make this or the jump above for [if]min.


On further inspection I don't think this is a problem because the GLSL 
rules say the loop condition must be scalar boolean.





-      if (!is_var_constant(limit))
+      if (limit->src.ssa->parent_instr->type !=
nir_instr_type_load_const)
           return false;

-      *limit_val =
nir_instr_as_load_const(limit->def->parent_instr)->value[0];
+      *limit_val =
+   
  nir_instr_as_load_const(limit->src.ssa->parent_instr)->value[0];


        terminator->exact_trip_count_unknown = true;

@@ -777,19 +778,19 @@
is_supported_terminator_condition(nir_alu_instr *alu)

  static bool
  get_induction_and_limit_vars(nir_alu_instr *alu, nir_loop_variable
**ind,
-                             nir_loop_variable **limit,
+                             nir_alu_src **limit,
                               loop_info_state *state)
  {
     bool limit_rhs = true;

     /* We assume that the limit is the "right" operand */
     *ind = get_loop_var(alu->src[0].src.ssa, state);
-   *limit = get_loop_var(alu->src[1].src.ssa, state);
+   *limit = >src[1];

     if ((*ind)->type != basic_induction) {
        /* We had it the wrong way, flip things around */
        *ind = get_loop_var(alu->src[1].src.ssa, state);
-      *limit = get_loop_var(alu->src[0].src.ssa, state);
+      *limit = >src[0];
        limit_rhs = false;
     }

@@ -799,7 +800,7 @@ get_induction_and_limit_vars(nir_alu_instr *alu,
nir_loop_variable **ind,
  static void
  try_find_trip_count_vars_in_iand(nir_alu_instr **alu,
                                   nir_loop_variable **ind,
-                                 nir_loop_variable **limit,
+                                 nir_alu_src **limit,
                                   bool *limit_rhs,
                                   loop_info_state *state)
  {
@@ -848,7 +849,7 @@ try_find_trip_count_vars_in_iand(nir_alu_instr
**alu,

     /* Try the other iand src if needed */
     if (*ind == NULL || (*ind && (*ind)->type != basic_induction) ||
-       !is_var_constant(*limit)) {
+       (*limit)->src.ssa->parent_instr->type !=
nir_instr_type_load_const) {
        src = iand->src[1].src.ssa;
        if (src->parent_instr->type == nir_instr_type_alu) {
           nir_alu_instr *tmp_alu = nir_instr_as_alu(src->parent_instr);
@@ -891,7 +892,7 @@ find_trip_count(loop_info_state *state)

        bool limit_rhs;
        nir_loop_variable *basic_ind = NULL;
-      nir_loop_variable *limit;
+      nir_alu_src *limit;
        if (alu->op == nir_op_inot || alu->op == 

Re: [Mesa-dev] [PATCH 1/2] nir/loop_analyze: used nir_alu_src to track loop limit

[Timothy Arceri]

On 20/6/19 5:57 am, Jason Ekstrand wrote:
On Wed, Jun 19, 2019 at 3:09 AM Timothy Arceri <mailto:tarc...@itsqueeze.com>> wrote:


This helps reduce the amount of abstraction in this pass and allows
us to retain more information about the src such as any swizzles.
Retaining the swizzle information is required for a bugfix in the
following patch.

Fixes: 6772a17acc8e ("nir: Add a loop analysis pass")
---
  src/compiler/nir/nir_loop_analyze.c | 37 +++--
  1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/src/compiler/nir/nir_loop_analyze.c
b/src/compiler/nir/nir_loop_analyze.c
index e85a404da1b..57d2d94cad2 100644
--- a/src/compiler/nir/nir_loop_analyze.c
+++ b/src/compiler/nir/nir_loop_analyze.c
@@ -543,25 +543,26 @@ guess_loop_limit(loop_info_state *state,
nir_const_value *limit_val,
  }

  static bool
-try_find_limit_of_alu(nir_loop_variable *limit, nir_const_value
*limit_val,
-                      nir_loop_terminator *terminator,
loop_info_state *state)
+try_find_limit_of_alu(nir_alu_src *limit, nir_const_value *limit_val,
+                      nir_loop_terminator *terminator)
  {
-   if(!is_var_alu(limit))
+   if(limit->src.ssa->parent_instr->type != nir_instr_type_alu)
        return false;

-   nir_alu_instr *limit_alu =
nir_instr_as_alu(limit->def->parent_instr);
+   nir_alu_instr *limit_alu =
nir_instr_as_alu(limit->src.ssa->parent_instr);

     if (limit_alu->op == nir_op_imin ||
         limit_alu->op == nir_op_fmin) {
-      limit = get_loop_var(limit_alu->src[0].src.ssa, state);
+      limit = _alu->src[0];

-      if (!is_var_constant(limit))
-         limit = get_loop_var(limit_alu->src[1].src.ssa, state);
+      if (limit->src.ssa->parent_instr->type !=
nir_instr_type_load_const)
+         limit = _alu->src[1];


This is still horribly broken w.r.t swizzles because we're not tracking 
the component as we make this or the jump above for [if]min.


I think we should probably just do a check to make sure the limit and 
invariant are scalar if we don't already. The rest of the pass cannot 
handle vectors anyway swizzles or not, and I don't think we should 
bother handling it either.






-      if (!is_var_constant(limit))
+      if (limit->src.ssa->parent_instr->type !=
nir_instr_type_load_const)
           return false;

-      *limit_val =
nir_instr_as_load_const(limit->def->parent_instr)->value[0];
+      *limit_val =
+   
  nir_instr_as_load_const(limit->src.ssa->parent_instr)->value[0];


        terminator->exact_trip_count_unknown = true;

@@ -777,19 +778,19 @@
is_supported_terminator_condition(nir_alu_instr *alu)

  static bool
  get_induction_and_limit_vars(nir_alu_instr *alu, nir_loop_variable
**ind,
-                             nir_loop_variable **limit,
+                             nir_alu_src **limit,
                               loop_info_state *state)
  {
     bool limit_rhs = true;

     /* We assume that the limit is the "right" operand */
     *ind = get_loop_var(alu->src[0].src.ssa, state);
-   *limit = get_loop_var(alu->src[1].src.ssa, state);
+   *limit = >src[1];

     if ((*ind)->type != basic_induction) {
        /* We had it the wrong way, flip things around */
        *ind = get_loop_var(alu->src[1].src.ssa, state);
-      *limit = get_loop_var(alu->src[0].src.ssa, state);
+      *limit = >src[0];
        limit_rhs = false;
     }

@@ -799,7 +800,7 @@ get_induction_and_limit_vars(nir_alu_instr *alu,
nir_loop_variable **ind,
  static void
  try_find_trip_count_vars_in_iand(nir_alu_instr **alu,
                                   nir_loop_variable **ind,
-                                 nir_loop_variable **limit,
+                                 nir_alu_src **limit,
                                   bool *limit_rhs,
                                   loop_info_state *state)
  {
@@ -848,7 +849,7 @@ try_find_trip_count_vars_in_iand(nir_alu_instr
**alu,

     /* Try the other iand src if needed */
     if (*ind == NULL || (*ind && (*ind)->type != basic_induction) ||
-       !is_var_constant(*limit)) {
+       (*limit)->src.ssa->parent_instr->type !=
nir_instr_type_load_const) {
        src = iand->src[1].src.ssa;
        if (src->parent_instr->type == nir_instr_type_alu) {
           nir_alu_instr *tmp_alu = nir_instr_as_alu(src->parent_instr);
@@ -891,7 +892,7 @@ find_trip_count(loop_info_state *state)

        bool limit_rhs;
        nir_loop_variable *basic_ind = NULL;
-      nir_loo

Re: [Mesa-dev] [PATCH 1/2] nir/loop_analyze: used nir_alu_src to track loop limit

[Timothy Arceri]

On 19/6/19 11:55 pm, Brian Paul wrote:

On 06/19/2019 02:08 AM, Timothy Arceri wrote:

This helps reduce the amount of abstraction in this pass and allows
us to retain more information about the src such as any swizzles.
Retaining the swizzle information is required for a bugfix in the
following patch.

Fixes: 6772a17acc8e ("nir: Add a loop analysis pass")
---
  src/compiler/nir/nir_loop_analyze.c | 37 +++--
  1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/src/compiler/nir/nir_loop_analyze.c 
b/src/compiler/nir/nir_loop_analyze.c

index e85a404da1b..57d2d94cad2 100644
--- a/src/compiler/nir/nir_loop_analyze.c
+++ b/src/compiler/nir/nir_loop_analyze.c
@@ -543,25 +543,26 @@ guess_loop_limit(loop_info_state *state, 
nir_const_value *limit_val,

  }
  static bool
-try_find_limit_of_alu(nir_loop_variable *limit, nir_const_value 
*limit_val,
-  nir_loop_terminator *terminator, 
loop_info_state *state)

+try_find_limit_of_alu(nir_alu_src *limit, nir_const_value *limit_val,
+  nir_loop_terminator *terminator)
  {
-   if(!is_var_alu(limit))
+   if(limit->src.ssa->parent_instr->type != nir_instr_type_alu)
    return false;
-   nir_alu_instr *limit_alu = 
nir_instr_as_alu(limit->def->parent_instr);
+   nir_alu_instr *limit_alu = 
nir_instr_as_alu(limit->src.ssa->parent_instr);

 if (limit_alu->op == nir_op_imin ||
 limit_alu->op == nir_op_fmin) {
-  limit = get_loop_var(limit_alu->src[0].src.ssa, state);
+  limit = _alu->src[0];
-  if (!is_var_constant(limit))
- limit = get_loop_var(limit_alu->src[1].src.ssa, state);
+  if (limit->src.ssa->parent_instr->type != 
nir_instr_type_load_const)

+ limit = _alu->src[1];
-  if (!is_var_constant(limit))
+  if (limit->src.ssa->parent_instr->type != 
nir_instr_type_load_const)

   return false;
-  *limit_val = 
nir_instr_as_load_const(limit->def->parent_instr)->value[0];

+  *limit_val =
+ 
nir_instr_as_load_const(limit->src.ssa->parent_instr)->value[0];

    terminator->exact_trip_count_unknown = true;
@@ -777,19 +778,19 @@ is_supported_terminator_condition(nir_alu_instr 
*alu)

  static bool
  get_induction_and_limit_vars(nir_alu_instr *alu, nir_loop_variable 
**ind,

- nir_loop_variable **limit,
+ nir_alu_src **limit,
   loop_info_state *state)
  {
 bool limit_rhs = true;
 /* We assume that the limit is the "right" operand */
 *ind = get_loop_var(alu->src[0].src.ssa, state);
-   *limit = get_loop_var(alu->src[1].src.ssa, state);
+   *limit = >src[1];
 if ((*ind)->type != basic_induction) {
    /* We had it the wrong way, flip things around */
    *ind = get_loop_var(alu->src[1].src.ssa, state);
-  *limit = get_loop_var(alu->src[0].src.ssa, state);
+  *limit = >src[0];
    limit_rhs = false;
 }
@@ -799,7 +800,7 @@ get_induction_and_limit_vars(nir_alu_instr *alu, 
nir_loop_variable **ind,

  static void
  try_find_trip_count_vars_in_iand(nir_alu_instr **alu,
   nir_loop_variable **ind,
- nir_loop_variable **limit,
+ nir_alu_src **limit,
   bool *limit_rhs,
   loop_info_state *state)
  {
@@ -848,7 +849,7 @@ try_find_trip_count_vars_in_iand(nir_alu_instr **alu,
 /* Try the other iand src if needed */
 if (*ind == NULL || (*ind && (*ind)->type != basic_induction) ||
-   !is_var_constant(*limit)) {
+   (*limit)->src.ssa->parent_instr->type != 
nir_instr_type_load_const) {

    src = iand->src[1].src.ssa;
    if (src->parent_instr->type == nir_instr_type_alu) {
   nir_alu_instr *tmp_alu = nir_instr_as_alu(src->parent_instr);
@@ -891,7 +892,7 @@ find_trip_count(loop_info_state *state)
    bool limit_rhs;
    nir_loop_variable *basic_ind = NULL;
-  nir_loop_variable *limit;
+  nir_alu_src *limit;
    if (alu->op == nir_op_inot || alu->op == nir_op_ieq) {
   nir_alu_instr *new_alu = alu;
   try_find_trip_count_vars_in_iand(_alu, _ind, ,
@@ -931,13 +932,13 @@ find_trip_count(loop_info_state *state)
    /* Attempt to find a constant limit for the loop */
    nir_const_value limit_val;
-  if (is_var_constant(limit)) {
+  if (limit->src.ssa->parent_instr->type == 
nir_instr_type_load_const) {

   limit_val =
-    nir_instr_as_load_const(limit->def->parent_instr)->value[0];
+
nir_instr_as_load_const(limit->src.ssa->parent_instr)->value[0];

    } else {
   trip_count_known = false;
- if (!try_find_limit_of_alu(limit, _val, terminator,

[Mesa-dev] [PATCH 2/2] nir/loop_analyze: handle swizzles on the loop limit

[Timothy Arceri]

Fixes: 6772a17acc8e ("nir: Add a loop analysis pass")
---
 src/compiler/nir/nir_loop_analyze.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/compiler/nir/nir_loop_analyze.c 
b/src/compiler/nir/nir_loop_analyze.c
index 57d2d94cad2..79ed5bb2712 100644
--- a/src/compiler/nir/nir_loop_analyze.c
+++ b/src/compiler/nir/nir_loop_analyze.c
@@ -561,8 +561,10 @@ try_find_limit_of_alu(nir_alu_src *limit, nir_const_value 
*limit_val,
   if (limit->src.ssa->parent_instr->type != nir_instr_type_load_const)
  return false;
 
+  unsigned limit_swz = limit->swizzle[0];
   *limit_val =
- nir_instr_as_load_const(limit->src.ssa->parent_instr)->value[0];
+ nir_instr_as_load_const(limit->src.ssa->parent_instr)
+->value[limit_swz];
 
   terminator->exact_trip_count_unknown = true;
 
@@ -933,8 +935,10 @@ find_trip_count(loop_info_state *state)
   /* Attempt to find a constant limit for the loop */
   nir_const_value limit_val;
   if (limit->src.ssa->parent_instr->type == nir_instr_type_load_const) {
+ unsigned limit_swz = limit->swizzle[0];
  limit_val =
-nir_instr_as_load_const(limit->src.ssa->parent_instr)->value[0];
+nir_instr_as_load_const(limit->src.ssa->parent_instr)
+   ->value[limit_swz];
   } else {
  trip_count_known = false;
 
-- 
2.21.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 1/2] nir/loop_analyze: used nir_alu_src to track loop limit

[Timothy Arceri]

This helps reduce the amount of abstraction in this pass and allows
us to retain more information about the src such as any swizzles.
Retaining the swizzle information is required for a bugfix in the
following patch.

Fixes: 6772a17acc8e ("nir: Add a loop analysis pass")
---
 src/compiler/nir/nir_loop_analyze.c | 37 +++--
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/src/compiler/nir/nir_loop_analyze.c 
b/src/compiler/nir/nir_loop_analyze.c
index e85a404da1b..57d2d94cad2 100644
--- a/src/compiler/nir/nir_loop_analyze.c
+++ b/src/compiler/nir/nir_loop_analyze.c
@@ -543,25 +543,26 @@ guess_loop_limit(loop_info_state *state, nir_const_value 
*limit_val,
 }
 
 static bool
-try_find_limit_of_alu(nir_loop_variable *limit, nir_const_value *limit_val,
-  nir_loop_terminator *terminator, loop_info_state *state)
+try_find_limit_of_alu(nir_alu_src *limit, nir_const_value *limit_val,
+  nir_loop_terminator *terminator)
 {
-   if(!is_var_alu(limit))
+   if(limit->src.ssa->parent_instr->type != nir_instr_type_alu)
   return false;
 
-   nir_alu_instr *limit_alu = nir_instr_as_alu(limit->def->parent_instr);
+   nir_alu_instr *limit_alu = nir_instr_as_alu(limit->src.ssa->parent_instr);
 
if (limit_alu->op == nir_op_imin ||
limit_alu->op == nir_op_fmin) {
-  limit = get_loop_var(limit_alu->src[0].src.ssa, state);
+  limit = _alu->src[0];
 
-  if (!is_var_constant(limit))
- limit = get_loop_var(limit_alu->src[1].src.ssa, state);
+  if (limit->src.ssa->parent_instr->type != nir_instr_type_load_const)
+ limit = _alu->src[1];
 
-  if (!is_var_constant(limit))
+  if (limit->src.ssa->parent_instr->type != nir_instr_type_load_const)
  return false;
 
-  *limit_val = nir_instr_as_load_const(limit->def->parent_instr)->value[0];
+  *limit_val =
+ nir_instr_as_load_const(limit->src.ssa->parent_instr)->value[0];
 
   terminator->exact_trip_count_unknown = true;
 
@@ -777,19 +778,19 @@ is_supported_terminator_condition(nir_alu_instr *alu)
 
 static bool
 get_induction_and_limit_vars(nir_alu_instr *alu, nir_loop_variable **ind,
- nir_loop_variable **limit,
+ nir_alu_src **limit,
  loop_info_state *state)
 {
bool limit_rhs = true;
 
/* We assume that the limit is the "right" operand */
*ind = get_loop_var(alu->src[0].src.ssa, state);
-   *limit = get_loop_var(alu->src[1].src.ssa, state);
+   *limit = >src[1];
 
if ((*ind)->type != basic_induction) {
   /* We had it the wrong way, flip things around */
   *ind = get_loop_var(alu->src[1].src.ssa, state);
-  *limit = get_loop_var(alu->src[0].src.ssa, state);
+  *limit = >src[0];
   limit_rhs = false;
}
 
@@ -799,7 +800,7 @@ get_induction_and_limit_vars(nir_alu_instr *alu, 
nir_loop_variable **ind,
 static void
 try_find_trip_count_vars_in_iand(nir_alu_instr **alu,
  nir_loop_variable **ind,
- nir_loop_variable **limit,
+ nir_alu_src **limit,
  bool *limit_rhs,
  loop_info_state *state)
 {
@@ -848,7 +849,7 @@ try_find_trip_count_vars_in_iand(nir_alu_instr **alu,
 
/* Try the other iand src if needed */
if (*ind == NULL || (*ind && (*ind)->type != basic_induction) ||
-   !is_var_constant(*limit)) {
+   (*limit)->src.ssa->parent_instr->type != nir_instr_type_load_const) {
   src = iand->src[1].src.ssa;
   if (src->parent_instr->type == nir_instr_type_alu) {
  nir_alu_instr *tmp_alu = nir_instr_as_alu(src->parent_instr);
@@ -891,7 +892,7 @@ find_trip_count(loop_info_state *state)
 
   bool limit_rhs;
   nir_loop_variable *basic_ind = NULL;
-  nir_loop_variable *limit;
+  nir_alu_src *limit;
   if (alu->op == nir_op_inot || alu->op == nir_op_ieq) {
  nir_alu_instr *new_alu = alu;
  try_find_trip_count_vars_in_iand(_alu, _ind, ,
@@ -931,13 +932,13 @@ find_trip_count(loop_info_state *state)
 
   /* Attempt to find a constant limit for the loop */
   nir_const_value limit_val;
-  if (is_var_constant(limit)) {
+  if (limit->src.ssa->parent_instr->type == nir_instr_type_load_const) {
  limit_val =
-nir_instr_as_load_const(limit->def->parent_instr)->value[0];
+nir_instr_as_load_const(limit->src.ssa->parent_instr)->value[0];
   } else {
  trip_count_known = false;
 
- if (!try_find_limit_of_alu(limit, _val, terminator, state)) {
+ if (!try_find_limit_of_alu(limit, _val, terminator)) {
 /* Guess loop limit based on array access */
 if (!guess_loop_limit(state, _val, basic_ind)) {
continue;
-- 
2.21.0

___
mesa-dev mailing list

[Mesa-dev] [PATCH 2/2] nir/loop_analyze: handle swizzles on invariant

[Timothy Arceri]

Fixes: 6772a17acc8e ("nir: Add a loop analysis pass")
---
 src/compiler/nir/nir_loop_analyze.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/compiler/nir/nir_loop_analyze.c 
b/src/compiler/nir/nir_loop_analyze.c
index ff73b32c51d..e85a404da1b 100644
--- a/src/compiler/nir/nir_loop_analyze.c
+++ b/src/compiler/nir/nir_loop_analyze.c
@@ -959,9 +959,10 @@ find_trip_count(loop_info_state *state)
  nir_instr_as_load_const(basic_ind->ind->def_outside_loop->
 def->parent_instr)->value;
 
+  unsigned invariant_swz = basic_ind->ind->invariant->swizzle[0];
   nir_const_value *step_val =
- nir_instr_as_load_const(basic_ind->ind->invariant->src.ssa->
- parent_instr)->value;
+ &(nir_instr_as_load_const(basic_ind->ind->invariant->src.ssa->
+   parent_instr)->value[invariant_swz]);
 
   int iterations = calculate_iterations(initial_val, step_val,
 _val,
-- 
2.21.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 1/2] nir/loop_analyze: use nir_src to track the invariant

[Timothy Arceri]

This helps reduce the amount of abstraction in this pass and allows
us to retain more information about the src such as any swizzles.
Retaining the swizzle information is required for a bugfix in a
following patch.

Fixes: 6772a17acc8e ("nir: Add a loop analysis pass")
---
 src/compiler/nir/nir_loop_analyze.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/compiler/nir/nir_loop_analyze.c 
b/src/compiler/nir/nir_loop_analyze.c
index 0ae9533e007..ff73b32c51d 100644
--- a/src/compiler/nir/nir_loop_analyze.c
+++ b/src/compiler/nir/nir_loop_analyze.c
@@ -60,7 +60,7 @@ typedef struct {
 typedef struct nir_basic_induction_var {
nir_op alu_op;   /* The type of alu-operation*/
nir_loop_variable *alu_def;  /* The def of the alu-operation */
-   nir_loop_variable *invariant;/* The invariant alu-operand*/
+   nir_alu_src *invariant;  /* The invariant alu-src*/
nir_loop_variable *def_outside_loop; /* The phi-src outside the loop */
 } nir_basic_induction_var;
 
@@ -356,7 +356,7 @@ compute_induction_information(loop_info_state *state)
   /* Is one of the operands const, and the other the phi */
   if (alu->src[i].src.ssa->parent_instr->type == 
nir_instr_type_load_const &&
   alu->src[1-i].src.ssa == >dest.ssa)
- biv->invariant = get_loop_var(alu->src[i].src.ssa, state);
+ biv->invariant = >src[i];
}
 }
  }
@@ -364,7 +364,7 @@ compute_induction_information(loop_info_state *state)
 
   if (biv->alu_def && biv->def_outside_loop && biv->invariant &&
   is_var_constant(biv->def_outside_loop)) {
- assert(is_var_constant(biv->invariant));
+ assert(is_var_constant(get_loop_var(biv->invariant->src.ssa, state)));
  biv->alu_def->type = basic_induction;
  biv->alu_def->ind = biv;
  var->type = basic_induction;
@@ -960,8 +960,8 @@ find_trip_count(loop_info_state *state)
 def->parent_instr)->value;
 
   nir_const_value *step_val =
- nir_instr_as_load_const(basic_ind->ind->invariant->def->
-parent_instr)->value;
+ nir_instr_as_load_const(basic_ind->ind->invariant->src.ssa->
+ parent_instr)->value;
 
   int iterations = calculate_iterations(initial_val, step_val,
 _val,
-- 
2.21.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] st/glsl: make sure to propagate initialisers to driver storage

[Timothy Arceri]

This essentially reverts 20234cfe3a20.

Fixes piglit test:
tests/spec/arb_get_program_binary/execution/uniform-after-restore.shader_test

Fixes: 20234cfe3a20 "st/mesa: don't propagate uniforms when restoring from 
cache"

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110784
---
 src/mesa/program/ir_to_mesa.cpp| 41 ++
 src/mesa/program/ir_to_mesa.h  |  3 +-
 src/mesa/state_tracker/st_glsl_to_nir.cpp  |  2 +-
 src/mesa/state_tracker/st_glsl_to_tgsi.cpp |  2 +-
 src/mesa/state_tracker/st_shader_cache.c   |  2 +-
 5 files changed, 23 insertions(+), 27 deletions(-)

diff --git a/src/mesa/program/ir_to_mesa.cpp b/src/mesa/program/ir_to_mesa.cpp
index f875c00238f..005b855230b 100644
--- a/src/mesa/program/ir_to_mesa.cpp
+++ b/src/mesa/program/ir_to_mesa.cpp
@@ -2506,8 +2506,7 @@ _mesa_generate_parameters_list_for_uniforms(struct 
gl_context *ctx,
 void
 _mesa_associate_uniform_storage(struct gl_context *ctx,
 struct gl_shader_program *shader_program,
-struct gl_program *prog,
-bool propagate_to_storage)
+struct gl_program *prog)
 {
struct gl_program_parameter_list *params = prog->Parameters;
gl_shader_stage shader_type = prog->info.stage;
@@ -2633,26 +2632,24 @@ _mesa_associate_uniform_storage(struct gl_context *ctx,
   * data from the linker's backing store.  This will cause values from
   * initializers in the source code to be copied over.
   */
- if (propagate_to_storage) {
-unsigned array_elements = MAX2(1, storage->array_elements);
-if (ctx->Const.PackedDriverUniformStorage && !prog->is_arb_asm &&
-(storage->is_bindless || !storage->type->contains_opaque())) {
-   const int dmul = storage->type->is_64bit() ? 2 : 1;
-   const unsigned components =
-  storage->type->vector_elements *
-  storage->type->matrix_columns;
-
-   for (unsigned s = 0; s < storage->num_driver_storage; s++) {
-  gl_constant_value *uni_storage = (gl_constant_value *)
- storage->driver_storage[s].data;
-  memcpy(uni_storage, storage->storage,
- sizeof(storage->storage[0]) * components *
- array_elements * dmul);
-   }
-} else {
-   _mesa_propagate_uniforms_to_driver_storage(storage, 0,
-  array_elements);
+ unsigned array_elements = MAX2(1, storage->array_elements);
+ if (ctx->Const.PackedDriverUniformStorage && !prog->is_arb_asm &&
+ (storage->is_bindless || !storage->type->contains_opaque())) {
+const int dmul = storage->type->is_64bit() ? 2 : 1;
+const unsigned components =
+   storage->type->vector_elements *
+   storage->type->matrix_columns;
+
+for (unsigned s = 0; s < storage->num_driver_storage; s++) {
+   gl_constant_value *uni_storage = (gl_constant_value *)
+  storage->driver_storage[s].data;
+   memcpy(uni_storage, storage->storage,
+  sizeof(storage->storage[0]) * components *
+  array_elements * dmul);
 }
+ } else {
+_mesa_propagate_uniforms_to_driver_storage(storage, 0,
+   array_elements);
  }
 
  last_location = location;
@@ -3011,7 +3008,7 @@ get_mesa_program(struct gl_context *ctx,
 * prog->ParameterValues to get reallocated (e.g., anything that adds a
 * program constant) has to happen before creating this linkage.
 */
-   _mesa_associate_uniform_storage(ctx, shader_program, prog, true);
+   _mesa_associate_uniform_storage(ctx, shader_program, prog);
if (!shader_program->data->LinkStatus) {
   goto fail_exit;
}
diff --git a/src/mesa/program/ir_to_mesa.h b/src/mesa/program/ir_to_mesa.h
index f5665e6316e..33eb801bae8 100644
--- a/src/mesa/program/ir_to_mesa.h
+++ b/src/mesa/program/ir_to_mesa.h
@@ -50,8 +50,7 @@ _mesa_generate_parameters_list_for_uniforms(struct gl_context 
*ctx,
 void
 _mesa_associate_uniform_storage(struct gl_context *ctx,
 struct gl_shader_program *shader_program,
-struct gl_program *prog,
-bool propagate_to_storage);
+struct gl_program *prog);
 
 #ifdef __cplusplus
 }
diff --git a/src/mesa/state_tracker/st_glsl_to_nir.cpp 
b/src/mesa/state_tracker/st_glsl_to_nir.cpp
index 11fc03baf86..b40cb9223df 100644
--- a/src/mesa/state_tracker/st_glsl_to_nir.cpp
+++ b/src/mesa/state_tracker/st_glsl_to_nir.cpp
@@ -524,7 +524,7 @@ st_glsl_to_nir_post_opts(struct st_context *st, struct 

[Mesa-dev] [PATCH] Revert "st/mesa: expose 0 shader binary formats for compat profiles for Qt"

[Timothy Arceri]

This reverts commit 55376cb31e2f495a4d872b4ffce2135c3365b873.

It's been over a year and both QT 5.9.5 and 5.11.0 contained a fix for the
original issue. It seems i965 only ever applied this workaround to the
18.0 branch.
---
 src/mesa/state_tracker/st_context.c|  2 +-
 src/mesa/state_tracker/st_extensions.c | 13 +++--
 src/mesa/state_tracker/st_extensions.h |  3 +--
 src/mesa/state_tracker/st_manager.c|  2 +-
 4 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/src/mesa/state_tracker/st_context.c 
b/src/mesa/state_tracker/st_context.c
index 8f2acafbca3..875be9d0029 100644
--- a/src/mesa/state_tracker/st_context.c
+++ b/src/mesa/state_tracker/st_context.c
@@ -645,7 +645,7 @@ st_create_context_priv(struct gl_context *ctx, struct 
pipe_context *pipe,
 
PIPE_CAP_MAX_TEXTURE_UPLOAD_MEMORY_BUDGET));
 
/* GL limits and extensions */
-   st_init_limits(pipe->screen, >Const, >Extensions, ctx->API);
+   st_init_limits(pipe->screen, >Const, >Extensions);
st_init_extensions(pipe->screen, >Const,
   >Extensions, >options, ctx->API);
 
diff --git a/src/mesa/state_tracker/st_extensions.c 
b/src/mesa/state_tracker/st_extensions.c
index 3d5b0fa5836..f930d3caff9 100644
--- a/src/mesa/state_tracker/st_extensions.c
+++ b/src/mesa/state_tracker/st_extensions.c
@@ -76,8 +76,7 @@ static int _clamp(int a, int min, int max)
  * Note that we have to limit/clamp against Mesa's internal limits too.
  */
 void st_init_limits(struct pipe_screen *screen,
-struct gl_constants *c, struct gl_extensions *extensions,
-gl_api api)
+struct gl_constants *c, struct gl_extensions *extensions)
 {
int supported_irs;
unsigned sh;
@@ -447,14 +446,8 @@ void st_init_limits(struct pipe_screen *screen,
c->GLSLFrontFacingIsSysVal =
   screen->get_param(screen, PIPE_CAP_TGSI_FS_FACE_IS_INTEGER_SYSVAL);
 
-   /* GL_ARB_get_program_binary
-*
-* The QT framework has a bug in their shader program cache, which is built
-* on GL_ARB_get_program_binary. In an effort to allow them to fix the bug
-* we don't enable more than 1 binary format for compatibility profiles.
-*/
-   if (api != API_OPENGL_COMPAT &&
-   screen->get_disk_shader_cache && screen->get_disk_shader_cache(screen))
+   /* GL_ARB_get_program_binary */
+   if (screen->get_disk_shader_cache && screen->get_disk_shader_cache(screen))
   c->NumProgramBinaryFormats = 1;
 
c->MaxAtomicBufferBindings =
diff --git a/src/mesa/state_tracker/st_extensions.h 
b/src/mesa/state_tracker/st_extensions.h
index fdfac7ece70..7bf1aa8c8cb 100644
--- a/src/mesa/state_tracker/st_extensions.h
+++ b/src/mesa/state_tracker/st_extensions.h
@@ -35,8 +35,7 @@ struct pipe_screen;
 
 extern void st_init_limits(struct pipe_screen *screen,
struct gl_constants *c,
-   struct gl_extensions *extensions,
-   gl_api api);
+   struct gl_extensions *extensions);
 
 extern void st_init_extensions(struct pipe_screen *screen,
struct gl_constants *consts,
diff --git a/src/mesa/state_tracker/st_manager.c 
b/src/mesa/state_tracker/st_manager.c
index 35d41f0a2c1..ff0bec8f569 100644
--- a/src/mesa/state_tracker/st_manager.c
+++ b/src/mesa/state_tracker/st_manager.c
@@ -1269,7 +1269,7 @@ get_version(struct pipe_screen *screen,
_mesa_init_constants(, api);
_mesa_init_extensions();
 
-   st_init_limits(screen, , , api);
+   st_init_limits(screen, , );
st_init_extensions(screen, , , options, api);
 
return _mesa_get_version(, , api);
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] radeonsi: add drirc workaround for American Truck Simulator

[Timothy Arceri]

Cc: "19.0" "19.1" 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110711
---
 src/util/00-mesa-defaults.conf | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/util/00-mesa-defaults.conf b/src/util/00-mesa-defaults.conf
index 3c459f3dec4..e36190f61c8 100644
--- a/src/util/00-mesa-defaults.conf
+++ b/src/util/00-mesa-defaults.conf
@@ -471,6 +471,9 @@ TODO: document the other workarounds.
 
 
 
+
+
+
 
 
 
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] util: add missing include to build_id.h

[Timothy Arceri]

Required to use uint8_t
---
 src/util/build_id.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/util/build_id.h b/src/util/build_id.h
index 86d611d8db7..1872ca5c7e5 100644
--- a/src/util/build_id.h
+++ b/src/util/build_id.h
@@ -26,6 +26,8 @@
 
 #ifdef HAVE_DL_ITERATE_PHDR
 
+#include 
+
 struct build_id_note;
 
 const struct build_id_note *
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] glsl_to_nir: remove unused type_is_int()

[Timothy Arceri]

This was missed in e00fa99b08b3.

Cc: Christian Gmeiner 
---
 src/compiler/glsl/glsl_to_nir.cpp | 9 -
 1 file changed, 9 deletions(-)

diff --git a/src/compiler/glsl/glsl_to_nir.cpp 
b/src/compiler/glsl/glsl_to_nir.cpp
index 47159ebd5e8..a51d39c4753 100644
--- a/src/compiler/glsl/glsl_to_nir.cpp
+++ b/src/compiler/glsl/glsl_to_nir.cpp
@@ -1728,15 +1728,6 @@ type_is_signed(glsl_base_type type)
   type == GLSL_TYPE_INT16;
 }
 
-static bool
-type_is_int(glsl_base_type type)
-{
-   return type == GLSL_TYPE_UINT || type == GLSL_TYPE_INT ||
-  type == GLSL_TYPE_UINT8 || type == GLSL_TYPE_INT8 ||
-  type == GLSL_TYPE_UINT16 || type == GLSL_TYPE_INT16 ||
-  type == GLSL_TYPE_UINT64 || type == GLSL_TYPE_INT64;
-}
-
 void
 nir_visitor::visit(ir_expression *ir)
 {
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radv: call constant folding before opt algebraic

[Timothy Arceri]

On 8/5/19 1:51 am, Samuel Pitoiset wrote:

What games are affected btw?


 PERCENTAGE DELTASShaders SGPRs VGPRs SpillSGPR CodeSize 
MaxWaves
 batman-arkham-city   2581 . . .. . 

 dawn-of-war-3 244 . . .. . 

 f1-2017  5627 . . .. . 

 fallout4-vr   196 . . .. . 

 nier 1905 . . .. . 

 no-mans-sky  4054 . . .. . 

 prey 2182 . . .. . 

 rot-tomb-raider  8391 . . .. . 

 skyrim-vr 494 . . .. . 

 sot-tomb-raider   613 . . .   0.01 % . 

 the_witcher_3-medium  803 . . .. . 


 the_wither_3-ultra   10400.05 %   -0.03 % .. 0.02 %
 valve-vr-pref-trace  323 . . .. .
 wolfenstein-2   1056 . .   -0.20 %  -0.02 % .

--
 All affected  690.50 %   -0.22 %   -7.69 %  -0.28   %0.57 %
--
 Total  29509 . .   -0.03 % . .



Can you please double check before pushing because of the flrp changes 
that landed around?


No change.



On 5/7/19 7:14 AM, Timothy Arceri wrote:

ping!

On 2/5/19 1:38 pm, Timothy Arceri wrote:

The pattern of calling opt algebraic first seems to have originated
in i965. The order in OpenGL drivers generally doesn't matter
because the GLSL IR optimisations do constant folding before
opt algebraic.

However in Vulkan drivers calling opt algebraic first can result
in missed constant folding opportunities.

vkpipeline-db results (VEGA64):

Totals from affected shaders:
SGPRS: 3160 -> 3176 (0.51 %)
VGPRS: 3588 -> 3580 (-0.22 %)
Spilled SGPRs: 52 -> 44 (-15.38 %)
Spilled VGPRs: 0 -> 0 (0.00 %)
Private memory VGPRs: 0 -> 0 (0.00 %)
Scratch size: 12 -> 12 (0.00 %) dwords per thread
Code Size: 261812 -> 261036 (-0.30 %) bytes
LDS: 7 -> 7 (0.00 %) blocks
Max Waves: 346 -> 348 (0.58 %)
Wait states: 0 -> 0 (0.00 %)
---
  src/amd/vulkan/radv_shader.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/amd/vulkan/radv_shader.c b/src/amd/vulkan/radv_shader.c
index cd5a9f2afb4..ad7b2439735 100644
--- a/src/amd/vulkan/radv_shader.c
+++ b/src/amd/vulkan/radv_shader.c
@@ -162,8 +162,8 @@ radv_optimize_nir(struct nir_shader *shader, bool 
optimize_conservatively,

  NIR_PASS(progress, shader, nir_opt_dead_cf);
  NIR_PASS(progress, shader, nir_opt_cse);
  NIR_PASS(progress, shader, nir_opt_peephole_select, 
8, true, true);

-    NIR_PASS(progress, shader, nir_opt_algebraic);
  NIR_PASS(progress, shader, nir_opt_constant_folding);
+    NIR_PASS(progress, shader, nir_opt_algebraic);
  NIR_PASS(progress, shader, nir_opt_undef);
  NIR_PASS(progress, shader, 
nir_opt_conditional_discard);

  if (shader->options->max_unroll_iterations) {


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] Revert "glx: Fix synthetic error generation in __glXSendError"

[Timothy Arceri]

On 7/5/19 6:27 pm, Michel Dänzer wrote:

On 2019-05-07 5:55 a.m., Timothy Arceri wrote:

This reverts commit e91ee763c378d03883eb88cf0eadd8aa916f7878.

This seems to have broken a number of wine games.

Cc: Adam Jackson 
Cc: Ian Romanick 
Cc: Hal Gentz 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110632
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110590
---
  src/glx/glx_error.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/glx/glx_error.c b/src/glx/glx_error.c
index 712ecf8213d..653cbeb2d2a 100644
--- a/src/glx/glx_error.c
+++ b/src/glx/glx_error.c
@@ -54,7 +54,7 @@ __glXSendError(Display * dpy, int_fast8_t errorCode, 
uint_fast32_t resourceID,
error.errorCode = glx_dpy->codes->first_error + errorCode;
 }
  
-   error.sequenceNumber = dpy->last_request_read;

+   error.sequenceNumber = dpy->request;
 error.resourceID = resourceID;
 error.minorCode = minorCode;
 error.majorCode = glx_dpy->majorOpcode;
@@ -73,7 +73,7 @@ __glXSendErrorForXcb(Display * dpy, const xcb_generic_error_t 
*err)
  
 error.type = X_Error;

 error.errorCode = err->error_code;
-   error.sequenceNumber = dpy->last_request_read;
+   error.sequenceNumber = err->sequence;
 error.resourceID = err->resource_id;
 error.minorCode = err->minor_code;
 error.majorCode = err->major_code;



As-is, this will re-introduce
https://bugs.freedesktop.org/show_bug.cgi?id=99781 . That one was about
__glXSendErrorForXcb, while the regressions are about __glXSendError, so
maybe only revert the __glXSendError hunk for now?




I don't know enough about this code to take responsibility for such 
changes. I was just trying to revert to the status quo until this could 
be investigated again.


My suggestion is we roll back the recent change. Then someone needs to 
create piglit test for both scenarios before trying to move forward again.


If you want to try something different then go for it :)
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radv: call constant folding before opt algebraic

[Timothy Arceri]

ping!

On 2/5/19 1:38 pm, Timothy Arceri wrote:

The pattern of calling opt algebraic first seems to have originated
in i965. The order in OpenGL drivers generally doesn't matter
because the GLSL IR optimisations do constant folding before
opt algebraic.

However in Vulkan drivers calling opt algebraic first can result
in missed constant folding opportunities.

vkpipeline-db results (VEGA64):

Totals from affected shaders:
SGPRS: 3160 -> 3176 (0.51 %)
VGPRS: 3588 -> 3580 (-0.22 %)
Spilled SGPRs: 52 -> 44 (-15.38 %)
Spilled VGPRs: 0 -> 0 (0.00 %)
Private memory VGPRs: 0 -> 0 (0.00 %)
Scratch size: 12 -> 12 (0.00 %) dwords per thread
Code Size: 261812 -> 261036 (-0.30 %) bytes
LDS: 7 -> 7 (0.00 %) blocks
Max Waves: 346 -> 348 (0.58 %)
Wait states: 0 -> 0 (0.00 %)
---
  src/amd/vulkan/radv_shader.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/amd/vulkan/radv_shader.c b/src/amd/vulkan/radv_shader.c
index cd5a9f2afb4..ad7b2439735 100644
--- a/src/amd/vulkan/radv_shader.c
+++ b/src/amd/vulkan/radv_shader.c
@@ -162,8 +162,8 @@ radv_optimize_nir(struct nir_shader *shader, bool 
optimize_conservatively,
  NIR_PASS(progress, shader, nir_opt_dead_cf);
  NIR_PASS(progress, shader, nir_opt_cse);
  NIR_PASS(progress, shader, nir_opt_peephole_select, 8, true, 
true);
-NIR_PASS(progress, shader, nir_opt_algebraic);
  NIR_PASS(progress, shader, nir_opt_constant_folding);
+NIR_PASS(progress, shader, nir_opt_algebraic);
  NIR_PASS(progress, shader, nir_opt_undef);
  NIR_PASS(progress, shader, nir_opt_conditional_discard);
  if (shader->options->max_unroll_iterations) {


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] Revert "glx: Fix synthetic error generation in __glXSendError"

[Timothy Arceri]

This reverts commit e91ee763c378d03883eb88cf0eadd8aa916f7878.

This seems to have broken a number of wine games.

Cc: Adam Jackson 
Cc: Ian Romanick 
Cc: Hal Gentz 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110632
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110590
---
 src/glx/glx_error.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/glx/glx_error.c b/src/glx/glx_error.c
index 712ecf8213d..653cbeb2d2a 100644
--- a/src/glx/glx_error.c
+++ b/src/glx/glx_error.c
@@ -54,7 +54,7 @@ __glXSendError(Display * dpy, int_fast8_t errorCode, 
uint_fast32_t resourceID,
   error.errorCode = glx_dpy->codes->first_error + errorCode;
}
 
-   error.sequenceNumber = dpy->last_request_read;
+   error.sequenceNumber = dpy->request;
error.resourceID = resourceID;
error.minorCode = minorCode;
error.majorCode = glx_dpy->majorOpcode;
@@ -73,7 +73,7 @@ __glXSendErrorForXcb(Display * dpy, const xcb_generic_error_t 
*err)
 
error.type = X_Error;
error.errorCode = err->error_code;
-   error.sequenceNumber = dpy->last_request_read;
+   error.sequenceNumber = err->sequence;
error.resourceID = err->resource_id;
error.minorCode = err->minor_code;
error.majorCode = err->major_code;
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] radeonsi: add an AMD_TEX_ANISO environment variable

[Timothy Arceri]

This brings it inline with the recently added AMD_DEBUG.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109619
---
 src/gallium/drivers/radeonsi/si_pipe.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/gallium/drivers/radeonsi/si_pipe.c 
b/src/gallium/drivers/radeonsi/si_pipe.c
index b0e0ca7af05..4d36fd46a9b 100644
--- a/src/gallium/drivers/radeonsi/si_pipe.c
+++ b/src/gallium/drivers/radeonsi/si_pipe.c
@@ -950,6 +950,10 @@ struct pipe_screen *radeonsi_screen_create(struct 
radeon_winsys *ws,
   sizeof(struct si_transfer), 64);
 
sscreen->force_aniso = MIN2(16, debug_get_num_option("R600_TEX_ANISO", 
-1));
+   if (sscreen->force_aniso == -1) {
+   sscreen->force_aniso = MIN2(16, 
debug_get_num_option("AMD_TEX_ANISO", -1));
+   }
+
if (sscreen->force_aniso >= 0) {
printf("radeonsi: Forcing anisotropy filter to %ix\n",
   /* round down to a power of two */
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] radeonsi: add config entry for Counter-Strike Global Offensive

[Timothy Arceri]

This fixes rendering issues with gun scopes which is rather
important.

Cc: "19.0" "19.1" 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100239
---
 src/util/00-mesa-defaults.conf | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/util/00-mesa-defaults.conf b/src/util/00-mesa-defaults.conf
index c704a1756ae..6389b796d33 100644
--- a/src/util/00-mesa-defaults.conf
+++ b/src/util/00-mesa-defaults.conf
@@ -468,6 +468,9 @@ TODO: document the other workarounds.
 
 
 
+
+
+
 
 
 
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] util/drirc: add workarounds for bugs in Doom 3: BFG

[Timothy Arceri]

This makes the game playable on radeonsi.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110143

Cc: "19.0" "19.1" 
---
 src/util/00-mesa-defaults.conf | 5 +
 1 file changed, 5 insertions(+)

diff --git a/src/util/00-mesa-defaults.conf b/src/util/00-mesa-defaults.conf
index f62315498b2..c704a1756ae 100644
--- a/src/util/00-mesa-defaults.conf
+++ b/src/util/00-mesa-defaults.conf
@@ -111,6 +111,11 @@ TODO: document the other workarounds.
 
 
 
+
+
+
+
+
 
 
 
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] radv: call constant folding before opt algebraic

[Timothy Arceri]

The pattern of calling opt algebraic first seems to have originated
in i965. The order in OpenGL drivers generally doesn't matter
because the GLSL IR optimisations do constant folding before
opt algebraic.

However in Vulkan drivers calling opt algebraic first can result
in missed constant folding opportunities.

vkpipeline-db results (VEGA64):

Totals from affected shaders:
SGPRS: 3160 -> 3176 (0.51 %)
VGPRS: 3588 -> 3580 (-0.22 %)
Spilled SGPRs: 52 -> 44 (-15.38 %)
Spilled VGPRs: 0 -> 0 (0.00 %)
Private memory VGPRs: 0 -> 0 (0.00 %)
Scratch size: 12 -> 12 (0.00 %) dwords per thread
Code Size: 261812 -> 261036 (-0.30 %) bytes
LDS: 7 -> 7 (0.00 %) blocks
Max Waves: 346 -> 348 (0.58 %)
Wait states: 0 -> 0 (0.00 %)
---
 src/amd/vulkan/radv_shader.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/amd/vulkan/radv_shader.c b/src/amd/vulkan/radv_shader.c
index cd5a9f2afb4..ad7b2439735 100644
--- a/src/amd/vulkan/radv_shader.c
+++ b/src/amd/vulkan/radv_shader.c
@@ -162,8 +162,8 @@ radv_optimize_nir(struct nir_shader *shader, bool 
optimize_conservatively,
 NIR_PASS(progress, shader, nir_opt_dead_cf);
 NIR_PASS(progress, shader, nir_opt_cse);
 NIR_PASS(progress, shader, nir_opt_peephole_select, 8, true, 
true);
-NIR_PASS(progress, shader, nir_opt_algebraic);
 NIR_PASS(progress, shader, nir_opt_constant_folding);
+NIR_PASS(progress, shader, nir_opt_algebraic);
 NIR_PASS(progress, shader, nir_opt_undef);
 NIR_PASS(progress, shader, nir_opt_conditional_discard);
 if (shader->options->max_unroll_iterations) {
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] glsl: fix and clean up NV_compute_shader_derivatives support

[Timothy Arceri]

I feel like I've asked this before but can you please try to add commit 
proper commit messages. Not adding a proper commit message might save 
you a minute or two but it cost much more than a minute or two for those 
trying to review a patch or that have bisect to the commit, used git 
blame, git log etc to find out why a change was made.


I started reviewing the patch when you first sent it but it wasn't 
immediately obvious what the fix was vs tidy ups. I didn't want to go 
looking over specs either, all the relevant information should have been 
here in the commit message.


Thanks,
Tim


On 2/5/19 11:17 am, Marek Olšák wrote:

Ping

On Wed, Apr 24, 2019 at 1:30 PM Marek Olšák > wrote:


From: Marek Olšák mailto:marek.ol...@amd.com>>

---
  src/compiler/glsl/builtin_functions.cpp | 78 -
  1 file changed, 24 insertions(+), 54 deletions(-)

diff --git a/src/compiler/glsl/builtin_functions.cpp
b/src/compiler/glsl/builtin_functions.cpp
index c8d9e1c9af3..b1ffafa1acf 100644
--- a/src/compiler/glsl/builtin_functions.cpp
+++ b/src/compiler/glsl/builtin_functions.cpp
@@ -125,23 +125,21 @@ gs_only(const _mesa_glsl_parse_state *state)
  static bool
  v110(const _mesa_glsl_parse_state *state)
  {
     return !state->es_shader;
  }

  static bool
  v110_derivatives_only(const _mesa_glsl_parse_state *state)
  {
     return !state->es_shader &&
-          (state->stage == MESA_SHADER_FRAGMENT ||
-           (state->stage == MESA_SHADER_COMPUTE &&
-            state->NV_compute_shader_derivatives_enable));
+          derivatives_only(state);
  }

  static bool
  v120(const _mesa_glsl_parse_state *state)
  {
     return state->is_version(120, 300);
  }

  static bool
  v130(const _mesa_glsl_parse_state *state)
@@ -158,38 +156,34 @@ v130_desktop(const _mesa_glsl_parse_state *state)
  static bool
  v460_desktop(const _mesa_glsl_parse_state *state)
  {
     return state->is_version(460, 0);
  }

  static bool
  v130_derivatives_only(const _mesa_glsl_parse_state *state)
  {
     return state->is_version(130, 300) &&
-          (state->stage == MESA_SHADER_FRAGMENT ||
-           (state->stage == MESA_SHADER_COMPUTE &&
-            state->NV_compute_shader_derivatives_enable));
+          derivatives_only(state);
  }

  static bool
  v140_or_es3(const _mesa_glsl_parse_state *state)
  {
     return state->is_version(140, 300);
  }

  static bool
  v400_derivatives_only(const _mesa_glsl_parse_state *state)
  {
     return state->is_version(400, 0) &&
-          (state->stage == MESA_SHADER_FRAGMENT ||
-           (state->stage == MESA_SHADER_COMPUTE &&
-            state->NV_compute_shader_derivatives_enable));
+          derivatives_only(state);
  }

  static bool
  texture_rectangle(const _mesa_glsl_parse_state *state)
  {
     return state->ARB_texture_rectangle_enable;
  }

  static bool
  texture_external(const _mesa_glsl_parse_state *state)
@@ -333,23 +327,21 @@ static bool
  gpu_shader4_tbo_integer(const _mesa_glsl_parse_state *state)
  {
     return gpu_shader4_tbo(state) &&
            state->ctx->Extensions.EXT_texture_integer;
  }

  static bool
  gpu_shader4_derivs_only(const _mesa_glsl_parse_state *state)
  {
     return state->EXT_gpu_shader4_enable &&
-          (state->stage == MESA_SHADER_FRAGMENT ||
-           (state->stage == MESA_SHADER_COMPUTE &&
-            state->NV_compute_shader_derivatives_enable));
+          derivatives_only(state);
  }

  static bool
  gpu_shader4_integer_derivs_only(const _mesa_glsl_parse_state *state)
  {
     return gpu_shader4_derivs_only(state) &&
            state->ctx->Extensions.EXT_texture_integer;
  }

  static bool
@@ -435,37 +427,35 @@ fs_interpolate_at(const _mesa_glsl_parse_state
*state)

  static bool
  texture_array_lod(const _mesa_glsl_parse_state *state)
  {
     return lod_exists_in_stage(state) &&
            (state->EXT_texture_array_enable ||
             (state->EXT_gpu_shader4_enable &&
              state->ctx->Extensions.EXT_texture_array));
  }

-static bool
-fs_texture_array(const _mesa_glsl_parse_state *state)
-{
-   return state->stage == MESA_SHADER_FRAGMENT &&
-          (state->EXT_texture_array_enable ||
-           (state->EXT_gpu_shader4_enable &&
-            state->ctx->Extensions.EXT_texture_array));
-}
-
  static bool
  texture_array(const _mesa_glsl_parse_state *state)
  {
     return state->EXT_texture_array_enable ||
            (state->EXT_gpu_shader4_enable &&
             state->ctx->Extensions.EXT_texture_array);
  }

+static 

Re: [Mesa-dev] [PATCH] nir: fix nir_remove_unused_varyings()

[Timothy Arceri]

On 26/4/19 6:50 am, Dylan Baker wrote:

Hi Tim,

I had to make a couple of small tweaks to get this to apply against 19.0 (namely
that the glsl_type_is_struct -> glsl_type_is_struct_or_ifc doesn't exist in
19.0), could you take a look at the patch in the staging/19.0 branch and let me
know if it looks okay?


Looks good to me. Thanks!



Thanks,
Dylan

Quoting Timothy Arceri (2019-04-24 18:17:42)

We were only setting the used mask for the first component of a
varying. Since the linking opts split vectors into scalars this
has mostly worked ok.

However this causes an issue where for example if we split a
struct on one side of the interface but not the other, then we
can possibly end up removing the first components on the side
that was split and then incorrectly remove the whole struct
on the other side of the varying.

With this change we simply mark all 4 components for each slot
used by a struct. We could possibly make this more fine gained
but that would require a more complex change.

This fixes a bug in Strange Brigade on RADV when tessellation
is enabled, all credit goes to Samuel Pitoiset for tracking down
the cause of the bug.

Fixes: f1eb5e639997 ("nir: add component level support to 
remove_unused_io_vars()")
---
  src/compiler/nir/nir_linking_helpers.c | 51 +-
  1 file changed, 33 insertions(+), 18 deletions(-)

diff --git a/src/compiler/nir/nir_linking_helpers.c 
b/src/compiler/nir/nir_linking_helpers.c
index f4494c78f98..ef0f94baf47 100644
--- a/src/compiler/nir/nir_linking_helpers.c
+++ b/src/compiler/nir/nir_linking_helpers.c
@@ -59,6 +59,15 @@ get_variable_io_mask(nir_variable *var, gl_shader_stage 
stage)
 return ((1ull << slots) - 1) << location;
  }
  
+static uint8_t

+get_num_components(nir_variable *var)
+{
+   if (glsl_type_is_struct_or_ifc(glsl_without_array(var->type)))
+  return 4;
+
+   return glsl_get_vector_elements(glsl_without_array(var->type));
+}
+
  static void
  tcs_add_output_reads(nir_shader *shader, uint64_t *read, uint64_t 
*patches_read)
  {
@@ -80,12 +89,14 @@ tcs_add_output_reads(nir_shader *shader, uint64_t *read, 
uint64_t *patches_read)
 continue;
  
  nir_variable *var = nir_deref_instr_get_variable(deref);

-if (var->data.patch) {
-   patches_read[var->data.location_frac] |=
-  get_variable_io_mask(var, shader->info.stage);
-} else {
-   read[var->data.location_frac] |=
-  get_variable_io_mask(var, shader->info.stage);
+for (unsigned i = 0; i < get_num_components(var); i++) {
+   if (var->data.patch) {
+  patches_read[var->data.location_frac + i] |=
+ get_variable_io_mask(var, shader->info.stage);
+   } else {
+  read[var->data.location_frac + i] |=
+ get_variable_io_mask(var, shader->info.stage);
+   }
  }
   }
}
@@ -161,22 +172,26 @@ nir_remove_unused_varyings(nir_shader *producer, 
nir_shader *consumer)
 uint64_t patches_read[4] = { 0 }, patches_written[4] = { 0 };
  
 nir_foreach_variable(var, >outputs) {

-  if (var->data.patch) {
- patches_written[var->data.location_frac] |=
-get_variable_io_mask(var, producer->info.stage);
-  } else {
- written[var->data.location_frac] |=
-get_variable_io_mask(var, producer->info.stage);
+  for (unsigned i = 0; i < get_num_components(var); i++) {
+ if (var->data.patch) {
+patches_written[var->data.location_frac + i] |=
+   get_variable_io_mask(var, producer->info.stage);
+ } else {
+written[var->data.location_frac + i] |=
+   get_variable_io_mask(var, producer->info.stage);
+ }
}
 }
  
 nir_foreach_variable(var, >inputs) {

-  if (var->data.patch) {
- patches_read[var->data.location_frac] |=
-get_variable_io_mask(var, consumer->info.stage);
-  } else {
- read[var->data.location_frac] |=
-get_variable_io_mask(var, consumer->info.stage);
+  for (unsigned i = 0; i < get_num_components(var); i++) {
+ if (var->data.patch) {
+patches_read[var->data.location_frac + i] |=
+   get_variable_io_mask(var, consumer->info.stage);
+ } else {
+read[var->data.location_frac + i] |=
+   get_variable_io_mask(var, consumer->info.stage);
+ }
}
 }
  
--

2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 1/2] radeonsi/nir: create si_nir_opts() helper

[Timothy Arceri]

We will make use of this in the following commit.
---
 src/gallium/drivers/radeonsi/si_shader.h |  1 +
 src/gallium/drivers/radeonsi/si_shader_nir.c | 78 +++-
 2 files changed, 43 insertions(+), 36 deletions(-)

diff --git a/src/gallium/drivers/radeonsi/si_shader.h 
b/src/gallium/drivers/radeonsi/si_shader.h
index f9f81a7bc1e..71ce27b2f55 100644
--- a/src/gallium/drivers/radeonsi/si_shader.h
+++ b/src/gallium/drivers/radeonsi/si_shader.h
@@ -710,6 +710,7 @@ void si_nir_scan_shader(const struct nir_shader *nir,
 void si_nir_scan_tess_ctrl(const struct nir_shader *nir,
   struct tgsi_tessctrl_info *out);
 void si_lower_nir(struct si_shader_selector *sel);
+void si_nir_opts(struct nir_shader *nir);
 
 /* Inline helpers. */
 
diff --git a/src/gallium/drivers/radeonsi/si_shader_nir.c 
b/src/gallium/drivers/radeonsi/si_shader_nir.c
index 4fe01ba0607..87100fbed19 100644
--- a/src/gallium/drivers/radeonsi/si_shader_nir.c
+++ b/src/gallium/drivers/radeonsi/si_shader_nir.c
@@ -811,6 +811,47 @@ void si_nir_scan_shader(const struct nir_shader *nir,
}
 }
 
+void
+si_nir_opts(struct nir_shader *nir)
+{
+   bool progress;
+   do {
+   progress = false;
+
+   NIR_PASS_V(nir, nir_lower_vars_to_ssa);
+
+   NIR_PASS(progress, nir, nir_opt_copy_prop_vars);
+   NIR_PASS(progress, nir, nir_opt_dead_write_vars);
+
+   NIR_PASS_V(nir, nir_lower_alu_to_scalar);
+   NIR_PASS_V(nir, nir_lower_phis_to_scalar);
+
+   /* (Constant) copy propagation is needed for txf with offsets. 
*/
+   NIR_PASS(progress, nir, nir_copy_prop);
+   NIR_PASS(progress, nir, nir_opt_remove_phis);
+   NIR_PASS(progress, nir, nir_opt_dce);
+   if (nir_opt_trivial_continues(nir)) {
+   progress = true;
+   NIR_PASS(progress, nir, nir_copy_prop);
+   NIR_PASS(progress, nir, nir_opt_dce);
+   }
+   NIR_PASS(progress, nir, nir_opt_if, true);
+   NIR_PASS(progress, nir, nir_opt_dead_cf);
+   NIR_PASS(progress, nir, nir_opt_cse);
+   NIR_PASS(progress, nir, nir_opt_peephole_select, 8, true, true);
+
+   /* Needed for algebraic lowering */
+   NIR_PASS(progress, nir, nir_opt_algebraic);
+   NIR_PASS(progress, nir, nir_opt_constant_folding);
+
+   NIR_PASS(progress, nir, nir_opt_undef);
+   NIR_PASS(progress, nir, nir_opt_conditional_discard);
+   if (nir->options->max_unroll_iterations) {
+   NIR_PASS(progress, nir, nir_opt_loop_unroll, 0);
+   }
+   } while (progress);
+}
+
 /**
  * Perform "lowering" operations on the NIR that are run once when the shader
  * selector is created.
@@ -861,42 +902,7 @@ si_lower_nir(struct si_shader_selector* sel)
 
ac_lower_indirect_derefs(sel->nir, sel->screen->info.chip_class);
 
-   bool progress;
-   do {
-   progress = false;
-
-   NIR_PASS_V(sel->nir, nir_lower_vars_to_ssa);
-
-   NIR_PASS(progress, sel->nir, nir_opt_copy_prop_vars);
-   NIR_PASS(progress, sel->nir, nir_opt_dead_write_vars);
-
-   NIR_PASS_V(sel->nir, nir_lower_alu_to_scalar);
-   NIR_PASS_V(sel->nir, nir_lower_phis_to_scalar);
-
-   /* (Constant) copy propagation is needed for txf with offsets. 
*/
-   NIR_PASS(progress, sel->nir, nir_copy_prop);
-   NIR_PASS(progress, sel->nir, nir_opt_remove_phis);
-   NIR_PASS(progress, sel->nir, nir_opt_dce);
-   if (nir_opt_trivial_continues(sel->nir)) {
-   progress = true;
-   NIR_PASS(progress, sel->nir, nir_copy_prop);
-   NIR_PASS(progress, sel->nir, nir_opt_dce);
-   }
-   NIR_PASS(progress, sel->nir, nir_opt_if, true);
-   NIR_PASS(progress, sel->nir, nir_opt_dead_cf);
-   NIR_PASS(progress, sel->nir, nir_opt_cse);
-   NIR_PASS(progress, sel->nir, nir_opt_peephole_select, 8, true, 
true);
-
-   /* Needed for algebraic lowering */
-   NIR_PASS(progress, sel->nir, nir_opt_algebraic);
-   NIR_PASS(progress, sel->nir, nir_opt_constant_folding);
-
-   NIR_PASS(progress, sel->nir, nir_opt_undef);
-   NIR_PASS(progress, sel->nir, nir_opt_conditional_discard);
-   if (sel->nir->options->max_unroll_iterations) {
-   NIR_PASS(progress, sel->nir, nir_opt_loop_unroll, 0);
-   }
-   } while (progress);
+   si_nir_opts(sel->nir);
 
NIR_PASS_V(sel->nir, nir_lower_bool_to_int32);
 
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org

[Mesa-dev] [PATCH 2/2] radeonsi/nir: call radeonsi nir opts before the scan pass

[Timothy Arceri]

Some of the opts are not called in the general optimastion loop
in the state trackers glsl -> nir conversion. We need to call
the radeonsi specific optimisation once before scanning over
the nir otherwise we can end up gathering info on code that
is later removed.

Fixes an assert in the piglit test:

./bin/varying-struct-centroid_gles3
---
 src/gallium/drivers/radeonsi/si_compute.c   | 1 +
 src/gallium/drivers/radeonsi/si_state_shaders.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/src/gallium/drivers/radeonsi/si_compute.c 
b/src/gallium/drivers/radeonsi/si_compute.c
index 541d7e6f118..f1a433b72df 100644
--- a/src/gallium/drivers/radeonsi/si_compute.c
+++ b/src/gallium/drivers/radeonsi/si_compute.c
@@ -106,6 +106,7 @@ static void si_create_compute_state_async(void *job, int 
thread_index)
assert(program->ir_type == PIPE_SHADER_IR_NIR);
sel.nir = program->ir.nir;
 
+   si_nir_opts(sel.nir);
si_nir_scan_shader(sel.nir, );
si_lower_nir();
}
diff --git a/src/gallium/drivers/radeonsi/si_state_shaders.c 
b/src/gallium/drivers/radeonsi/si_state_shaders.c
index 5bdfd4f6ac1..7250b40c5db 100644
--- a/src/gallium/drivers/radeonsi/si_state_shaders.c
+++ b/src/gallium/drivers/radeonsi/si_state_shaders.c
@@ -2240,6 +2240,7 @@ static void *si_create_shader_selector(struct 
pipe_context *ctx,
 
sel->nir = state->ir.nir;
 
+   si_nir_opts(sel->nir);
si_nir_scan_shader(sel->nir, >info);
si_nir_scan_tess_ctrl(sel->nir, >tcs_info);
}
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] nir: fix nir_remove_unused_varyings()

[Timothy Arceri]

We were only setting the used mask for the first component of a
varying. Since the linking opts split vectors into scalars this
has mostly worked ok.

However this causes an issue where for example if we split a
struct on one side of the interface but not the other, then we
can possibly end up removing the first components on the side
that was split and then incorrectly remove the whole struct
on the other side of the varying.

With this change we simply mark all 4 components for each slot
used by a struct. We could possibly make this more fine gained
but that would require a more complex change.

This fixes a bug in Strange Brigade on RADV when tessellation
is enabled, all credit goes to Samuel Pitoiset for tracking down
the cause of the bug.

Fixes: f1eb5e639997 ("nir: add component level support to 
remove_unused_io_vars()")
---
 src/compiler/nir/nir_linking_helpers.c | 51 +-
 1 file changed, 33 insertions(+), 18 deletions(-)

diff --git a/src/compiler/nir/nir_linking_helpers.c 
b/src/compiler/nir/nir_linking_helpers.c
index f4494c78f98..ef0f94baf47 100644
--- a/src/compiler/nir/nir_linking_helpers.c
+++ b/src/compiler/nir/nir_linking_helpers.c
@@ -59,6 +59,15 @@ get_variable_io_mask(nir_variable *var, gl_shader_stage 
stage)
return ((1ull << slots) - 1) << location;
 }
 
+static uint8_t
+get_num_components(nir_variable *var)
+{
+   if (glsl_type_is_struct_or_ifc(glsl_without_array(var->type)))
+  return 4;
+
+   return glsl_get_vector_elements(glsl_without_array(var->type));
+}
+
 static void
 tcs_add_output_reads(nir_shader *shader, uint64_t *read, uint64_t 
*patches_read)
 {
@@ -80,12 +89,14 @@ tcs_add_output_reads(nir_shader *shader, uint64_t *read, 
uint64_t *patches_read)
continue;
 
 nir_variable *var = nir_deref_instr_get_variable(deref);
-if (var->data.patch) {
-   patches_read[var->data.location_frac] |=
-  get_variable_io_mask(var, shader->info.stage);
-} else {
-   read[var->data.location_frac] |=
-  get_variable_io_mask(var, shader->info.stage);
+for (unsigned i = 0; i < get_num_components(var); i++) {
+   if (var->data.patch) {
+  patches_read[var->data.location_frac + i] |=
+ get_variable_io_mask(var, shader->info.stage);
+   } else {
+  read[var->data.location_frac + i] |=
+ get_variable_io_mask(var, shader->info.stage);
+   }
 }
  }
   }
@@ -161,22 +172,26 @@ nir_remove_unused_varyings(nir_shader *producer, 
nir_shader *consumer)
uint64_t patches_read[4] = { 0 }, patches_written[4] = { 0 };
 
nir_foreach_variable(var, >outputs) {
-  if (var->data.patch) {
- patches_written[var->data.location_frac] |=
-get_variable_io_mask(var, producer->info.stage);
-  } else {
- written[var->data.location_frac] |=
-get_variable_io_mask(var, producer->info.stage);
+  for (unsigned i = 0; i < get_num_components(var); i++) {
+ if (var->data.patch) {
+patches_written[var->data.location_frac + i] |=
+   get_variable_io_mask(var, producer->info.stage);
+ } else {
+written[var->data.location_frac + i] |=
+   get_variable_io_mask(var, producer->info.stage);
+ }
   }
}
 
nir_foreach_variable(var, >inputs) {
-  if (var->data.patch) {
- patches_read[var->data.location_frac] |=
-get_variable_io_mask(var, consumer->info.stage);
-  } else {
- read[var->data.location_frac] |=
-get_variable_io_mask(var, consumer->info.stage);
+  for (unsigned i = 0; i < get_num_components(var); i++) {
+ if (var->data.patch) {
+patches_read[var->data.location_frac + i] |=
+   get_variable_io_mask(var, consumer->info.stage);
+ } else {
+read[var->data.location_frac + i] |=
+   get_variable_io_mask(var, consumer->info.stage);
+ }
   }
}
 
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] nir: add an option for skipping split_alu_of_phi

[Timothy Arceri]

On 24/4/19 10:59 am, Jason Ekstrand wrote:
On Tue, Apr 23, 2019 at 7:39 PM Timothy Arceri <mailto:tarc...@itsqueeze.com>> wrote:


On 24/4/19 1:45 am, Samuel Pitoiset wrote:
 >
 > On 4/23/19 5:16 PM, Jason Ekstrand wrote:
 >> On Tue, Apr 23, 2019 at 7:46 AM Samuel Pitoiset
 >> mailto:samuel.pitoi...@gmail.com>
<mailto:samuel.pitoi...@gmail.com
<mailto:samuel.pitoi...@gmail.com>>> wrote:
 >>
 >>
 >>     On 4/23/19 10:45 AM, Bas Nieuwenhuizen wrote:
 >>     > On Tue, Apr 23, 2019 at 9:35 AM Samuel Pitoiset
 >>     > mailto:samuel.pitoi...@gmail.com> <mailto:samuel.pitoi...@gmail.com
<mailto:samuel.pitoi...@gmail.com>>>
 >>     wrote:
 >>     >> Signed-off-by: Samuel Pitoiset mailto:samuel.pitoi...@gmail.com>
 >>     <mailto:samuel.pitoi...@gmail.com
<mailto:samuel.pitoi...@gmail.com>>>
 >>     >> ---
 >>     >>   src/amd/vulkan/radv_shader.c                 |  2 +-
 >>     >>   src/compiler/nir/nir.h                       |  3 ++-
 >>     >>   src/compiler/nir/nir_opt_if.c                | 17
 >>     ++---
 >>     >>   src/freedreno/ir3/ir3_nir.c                  |  2 +-
 >>     >>   src/gallium/auxiliary/nir/tgsi_to_nir.c      |  2 +-
 >>     >>   src/gallium/drivers/freedreno/a2xx/ir2_nir.c |  2 +-
 >>     >>   src/gallium/drivers/radeonsi/si_shader_nir.c |  2 +-
 >>     >>   src/intel/compiler/brw_nir.c                 |  2 +-
 >>     >>   src/mesa/state_tracker/st_glsl_to_nir.cpp    |  2 +-
 >>     >>   9 files changed, 19 insertions(+), 15 deletions(-)
 >>     >>
 >>     >> diff --git a/src/amd/vulkan/radv_shader.c
 >>     b/src/amd/vulkan/radv_shader.c
 >>     >> index 13f1f9aa9dc..54a4e732230 100644
 >>     >> --- a/src/amd/vulkan/radv_shader.c
 >>     >> +++ b/src/amd/vulkan/radv_shader.c
 >>     >> @@ -158,7 +158,7 @@ radv_optimize_nir(struct nir_shader
 >>     *shader, bool optimize_conservatively,
 >>     >>                          NIR_PASS(progress, shader,
 >>     nir_opt_remove_phis);
 >>     >>                           NIR_PASS(progress, shader,
nir_opt_dce);
 >>     >>                   }
 >>     >> -                NIR_PASS(progress, shader, nir_opt_if,
true);
 >>     >> +                NIR_PASS(progress, shader, nir_opt_if, true,
 >>     false);
 >>     >>                   NIR_PASS(progress, shader,
nir_opt_dead_cf);
 >>     >>                   NIR_PASS(progress, shader, nir_opt_cse);
 >>     >>                   NIR_PASS(progress, shader,
 >>     nir_opt_peephole_select, 8, true, true);
 >>     >> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
 >>     >> index 7d2062d3691..d7506d6ddd1 100644
 >>     >> --- a/src/compiler/nir/nir.h
 >>     >> +++ b/src/compiler/nir/nir.h
 >>     >> @@ -3474,7 +3474,8 @@ bool nir_opt_gcm(nir_shader
*shader, bool
 >>     value_number);
 >>     >>
 >>     >>   bool nir_opt_idiv_const(nir_shader *shader, unsigned
 >>     min_bit_size);
 >>     >>
 >>     >> -bool nir_opt_if(nir_shader *shader, bool
 >>     aggressive_last_continue);
 >>     >> +bool nir_opt_if(nir_shader *shader, bool
aggressive_last_continue,
 >>     >> +                bool skip_alu_of_phi);
 >>     > Can we have a flag for this instead (e.g. something like
 >>     > nir_opt_if_skip_alu_of_phi)? I think have a function with
a bunch of
 >>     > bools is less than ideal as you can't see at the calling site
 >>     what is
 >>     > for what arg.
 >>     Yes, that seems better to me.
 >>
 >>
 >> This is the worst kind of hack all around.  We're making NIR more
 >> complicated and adding a flag to disable a useful and correct
piece of
 >> an optimization, not because it causes a perf regression but
because
 >> the back-end compiler is broken and this is easier than fixing it
 >> properly. Seriously?  Can't we just fix the LLVM back-end?  Or, if
 >> this optimization is actually doing something wrong, fix it?  Or
maybe
 >> actually figure out what pattern is causing LLVM to fall over
  

Re: [Mesa-dev] [PATCH 1/2] nir: add an option for skipping split_alu_of_phi

[Timothy Arceri]

On 24/4/19 1:45 am, Samuel Pitoiset wrote:


On 4/23/19 5:16 PM, Jason Ekstrand wrote:
On Tue, Apr 23, 2019 at 7:46 AM Samuel Pitoiset 
mailto:samuel.pitoi...@gmail.com>> wrote:



On 4/23/19 10:45 AM, Bas Nieuwenhuizen wrote:
> On Tue, Apr 23, 2019 at 9:35 AM Samuel Pitoiset
> mailto:samuel.pitoi...@gmail.com>>
wrote:
>> Signed-off-by: Samuel Pitoiset mailto:samuel.pitoi...@gmail.com>>
>> ---
>>   src/amd/vulkan/radv_shader.c                 |  2 +-
>>   src/compiler/nir/nir.h                       |  3 ++-
>>   src/compiler/nir/nir_opt_if.c                | 17
++---
>>   src/freedreno/ir3/ir3_nir.c                  |  2 +-
>>   src/gallium/auxiliary/nir/tgsi_to_nir.c      |  2 +-
>>   src/gallium/drivers/freedreno/a2xx/ir2_nir.c |  2 +-
>>   src/gallium/drivers/radeonsi/si_shader_nir.c |  2 +-
>>   src/intel/compiler/brw_nir.c                 |  2 +-
>>   src/mesa/state_tracker/st_glsl_to_nir.cpp    |  2 +-
>>   9 files changed, 19 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/amd/vulkan/radv_shader.c
b/src/amd/vulkan/radv_shader.c
>> index 13f1f9aa9dc..54a4e732230 100644
>> --- a/src/amd/vulkan/radv_shader.c
>> +++ b/src/amd/vulkan/radv_shader.c
>> @@ -158,7 +158,7 @@ radv_optimize_nir(struct nir_shader
*shader, bool optimize_conservatively,
>>                          NIR_PASS(progress, shader,
nir_opt_remove_phis);
>>                           NIR_PASS(progress, shader, nir_opt_dce);
>>                   }
>> -                NIR_PASS(progress, shader, nir_opt_if, true);
>> +                NIR_PASS(progress, shader, nir_opt_if, true,
false);
>>                   NIR_PASS(progress, shader, nir_opt_dead_cf);
>>                   NIR_PASS(progress, shader, nir_opt_cse);
>>                   NIR_PASS(progress, shader,
nir_opt_peephole_select, 8, true, true);
>> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
>> index 7d2062d3691..d7506d6ddd1 100644
>> --- a/src/compiler/nir/nir.h
>> +++ b/src/compiler/nir/nir.h
>> @@ -3474,7 +3474,8 @@ bool nir_opt_gcm(nir_shader *shader, bool
value_number);
>>
>>   bool nir_opt_idiv_const(nir_shader *shader, unsigned
min_bit_size);
>>
>> -bool nir_opt_if(nir_shader *shader, bool
aggressive_last_continue);
>> +bool nir_opt_if(nir_shader *shader, bool aggressive_last_continue,
>> +                bool skip_alu_of_phi);
> Can we have a flag for this instead (e.g. something like
> nir_opt_if_skip_alu_of_phi)? I think have a function with a bunch of
> bools is less than ideal as you can't see at the calling site
what is
> for what arg.
Yes, that seems better to me.


This is the worst kind of hack all around.  We're making NIR more 
complicated and adding a flag to disable a useful and correct piece of 
an optimization, not because it causes a perf regression but because 
the back-end compiler is broken and this is easier than fixing it 
properly. Seriously?  Can't we just fix the LLVM back-end?  Or, if 
this optimization is actually doing something wrong, fix it?  Or maybe 
actually figure out what pattern is causing LLVM to fall over and have 
a hack in your NIR -> LLVM pass?  On the list of "good ways to fix 
this problem", this seems to be pretty far down if it hasn't fallen 
off the bottom.


Best hack of the month? :-)

As discussed over IRC, this is definitely not the best solution, I don't 
like it either as I said.


I will work on a different solution maybe in our NIR->LLVM pass.

The aggressive_last_continue option should also be removed (make it 
default?).


The aggressive_last_continue option is not a hack to avoid a bug in a 
driver backend it is to avoid perf regressions. That said we may be able 
to remove it now that Jason has landed 
cd4ffb376f2aeefdd6a1b80d69a1580c4e569778






--Jason

>>   bool nir_opt_intrinsics(nir_shader *shader);
>>
>> diff --git a/src/compiler/nir/nir_opt_if.c
b/src/compiler/nir/nir_opt_if.c
>> index f674185f1e2..149b3bd1659 100644
>> --- a/src/compiler/nir/nir_opt_if.c
>> +++ b/src/compiler/nir/nir_opt_if.c
>> @@ -1385,7 +1385,8 @@ opt_if_cf_list(nir_builder *b, struct
exec_list *cf_list,
>>    * not do anything to cause the metadata to become invalid.
>>    */
>>   static bool
>> -opt_if_safe_cf_list(nir_builder *b, struct exec_list *cf_list)
>> +opt_if_safe_cf_list(nir_builder *b, struct exec_list *cf_list,
>> +                    bool skip_alu_of_phi)
>>   {
>>      bool progress = false;
>>      foreach_list_typed(nir_cf_node, cf_node, node, cf_list) {
>> @@ -1395,16 +1396,17 @@ opt_if_safe_cf_list(nir_builder *b,
struct exec_list *cf_list)
>>
>>         case nir_cf_node_if: {
>>            nir_if *nif = nir_cf_node_as_if(cf_node);
>> -         progress |= 

[Mesa-dev] [PATCH] st/mesa/radeonsi: fix race between destruction of types and shader compilation

[Timothy Arceri]

Commit 624789e3708c moved the destruction of types out of atexit() and
made use of a ref count instead. This is useful for avoiding a crash
where drivers such as radeonsi are still compiling in a thread when the app
exits and has not called MakeCurrent to change from the current context.

While the above scenario is technically an app bug we shouldn't crash.
However that change caused another race condition between the shader
compilation tread in radeonsi and context teardown functions.

This patch makes two changes to fix this new problem:

First we explicitly call _mesa_destroy_shader_compiler_types() when destroying
the st context rather than calling it indirectly via _mesa_free_context_data().
We do this as we must call it after st_destroy_context_priv() so that we don't
destory the glsl types before the compilation threads finish.

Next wait for the shader threads to finish in si_destroy_context() this
also means we need to call context destroy before destroying the queues
in si_destroy_screen().

Fixes: 624789e3708c ("compiler/glsl: handle case where we have multiple users 
for types")
---
 src/compiler/glsl/glsl_parser_extras.h  | 1 +
 src/gallium/drivers/radeonsi/si_pipe.c  | 8 ++--
 src/mesa/drivers/dri/i915/intel_context.c   | 2 +-
 src/mesa/drivers/dri/i965/brw_context.c | 2 +-
 src/mesa/drivers/dri/nouveau/nouveau_context.c  | 2 +-
 src/mesa/drivers/dri/radeon/radeon_common_context.c | 2 +-
 src/mesa/drivers/osmesa/osmesa.c| 8 
 src/mesa/drivers/x11/xm_api.c   | 4 ++--
 src/mesa/main/context.c | 7 ---
 src/mesa/main/context.h | 2 +-
 src/mesa/state_tracker/st_context.c | 8 +++-
 11 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/src/compiler/glsl/glsl_parser_extras.h 
b/src/compiler/glsl/glsl_parser_extras.h
index f92d2160aac..edc6fc06c77 100644
--- a/src/compiler/glsl/glsl_parser_extras.h
+++ b/src/compiler/glsl/glsl_parser_extras.h
@@ -997,6 +997,7 @@ extern "C" {
 #endif
 
 struct glcpp_parser;
+struct _mesa_glsl_parse_state;
 
 typedef void (*glcpp_extension_iterator)(
   struct _mesa_glsl_parse_state *state,
diff --git a/src/gallium/drivers/radeonsi/si_pipe.c 
b/src/gallium/drivers/radeonsi/si_pipe.c
index fa96ce34224..e9e1bd0aa38 100644
--- a/src/gallium/drivers/radeonsi/si_pipe.c
+++ b/src/gallium/drivers/radeonsi/si_pipe.c
@@ -150,6 +150,9 @@ static void si_destroy_context(struct pipe_context *context)
struct si_context *sctx = (struct si_context *)context;
int i;
 
+   util_queue_finish(>screen->shader_compiler_queue);
+   util_queue_finish(>screen->shader_compiler_queue_low_priority);
+
/* Unreference the framebuffer normally to disable related logic
 * properly.
 */
@@ -702,6 +705,9 @@ static void si_destroy_screen(struct pipe_screen* pscreen)
if (!sscreen->ws->unref(sscreen->ws))
return;
 
+   mtx_destroy(>aux_context_lock);
+   sscreen->aux_context->destroy(sscreen->aux_context);
+
util_queue_destroy(>shader_compiler_queue);
util_queue_destroy(>shader_compiler_queue_low_priority);
 
@@ -728,8 +734,6 @@ static void si_destroy_screen(struct pipe_screen* pscreen)
si_gpu_load_kill_thread(sscreen);
 
mtx_destroy(>gpu_load_mutex);
-   mtx_destroy(>aux_context_lock);
-   sscreen->aux_context->destroy(sscreen->aux_context);
 
slab_destroy_parent(>pool_transfers);
 
diff --git a/src/mesa/drivers/dri/i915/intel_context.c 
b/src/mesa/drivers/dri/i915/intel_context.c
index c23e5ffb26e..aa3175816cf 100644
--- a/src/mesa/drivers/dri/i915/intel_context.c
+++ b/src/mesa/drivers/dri/i915/intel_context.c
@@ -599,7 +599,7 @@ intelDestroyContext(__DRIcontext * driContextPriv)
   driDestroyOptionCache(>optionCache);
 
   /* free the Mesa context */
-  _mesa_free_context_data(>ctx);
+  _mesa_free_context_data(>ctx, true);
 
   _math_matrix_dtr(>ViewportMatrix);
 
diff --git a/src/mesa/drivers/dri/i965/brw_context.c 
b/src/mesa/drivers/dri/i965/brw_context.c
index ab637ddf721..df12f373f22 100644
--- a/src/mesa/drivers/dri/i965/brw_context.c
+++ b/src/mesa/drivers/dri/i965/brw_context.c
@@ -1226,7 +1226,7 @@ intelDestroyContext(__DRIcontext * driContextPriv)
driDestroyOptionCache(>optionCache);
 
/* free the Mesa context */
-   _mesa_free_context_data(>ctx);
+   _mesa_free_context_data(>ctx, true);
 
ralloc_free(brw);
driContextPriv->driverPrivate = NULL;
diff --git a/src/mesa/drivers/dri/nouveau/nouveau_context.c 
b/src/mesa/drivers/dri/nouveau/nouveau_context.c
index 93f6ce473a1..8fec35237c0 100644
--- a/src/mesa/drivers/dri/nouveau/nouveau_context.c
+++ b/src/mesa/drivers/dri/nouveau/nouveau_context.c
@@ -217,7 +217,7 @@ nouveau_context_deinit(struct gl_context *ctx)
nouveau_object_del(>hw.chan);
 

Re: [Mesa-dev] [PATCH 3/3] ac/nir: use the new raw/struct SSBO atomic intrisics for comp_swap

[Timothy Arceri]

Series:

Reviewed-by: Timothy Arceri 

On 18/4/19 5:23 pm, Samuel Pitoiset wrote:

This is actually fixed now.

This change requires LLVM r358579. Make sure to have it in
your tree, otherwise the following piglit will hang:

tests/spec/arb_shader_storage_buffer_object/execution/ssbo-atomicCompSwap-int.shader_test

Signed-off-by: Samuel Pitoiset 
---
  src/amd/common/ac_nir_to_llvm.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/amd/common/ac_nir_to_llvm.c b/src/amd/common/ac_nir_to_llvm.c
index 30e5cc8c389..78f25b8e742 100644
--- a/src/amd/common/ac_nir_to_llvm.c
+++ b/src/amd/common/ac_nir_to_llvm.c
@@ -1692,8 +1692,7 @@ static LLVMValueRef visit_atomic_ssbo(struct 
ac_nir_context *ctx,
  get_src(ctx, instr->src[0]),
  true);
  
-	if (HAVE_LLVM >= 0x900 &&

-   instr->intrinsic != nir_intrinsic_ssbo_atomic_comp_swap) {
+   if (HAVE_LLVM >= 0x900) {
/* XXX: The new raw/struct atomic intrinsics are buggy with
 * LLVM 8, see r358579.
 */


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/3] Revert "radv: add VK_KHR_shader_atomic_int64 but disable it for now"

[Timothy Arceri]

On 18/4/19 4:39 pm, Samuel Pitoiset wrote:


On 4/18/19 8:38 AM, Samuel Pitoiset wrote:


On 4/18/19 8:34 AM, Timothy Arceri wrote:

On 18/4/19 4:34 pm, Samuel Pitoiset wrote:


On 4/18/19 8:16 AM, Timothy Arceri wrote:

Meant to add this was tested on LLVM 9.


What do you mean?


I meant to say in the commit messages:

This first patch in this series caused a piglit regression with 
radeonsi NIR on my VEGA64 with LLVM 9.


Well, there is something really strange with RadeonSI NIR then.


??

It should just work...


I wonder if https://reviews.llvm.org/D60731 fixed the issue.

Do you have in your tree?


I didn't. Can confirm that fixes it, please ignore the series.









On 18/4/19 4:15 pm, Timothy Arceri wrote:

This reverts commit 9cf55b022dfa43f8fe3163edeb87a1c25ebf5a16.

This first patch in this series caused a piglit regression with
radeonsi NIR on my VEGA64.

tests/spec/arb_shader_storage_buffer_object/execution/ssbo-atomicAdd-int.shader_test 


---
  src/amd/vulkan/radv_device.c  | 10 --
  src/amd/vulkan/radv_extensions.py |  1 -
  src/amd/vulkan/radv_shader.c  |  1 -
  3 files changed, 12 deletions(-)

diff --git a/src/amd/vulkan/radv_device.c 
b/src/amd/vulkan/radv_device.c

index 13021a9f2da..1f77dcadb17 100644
--- a/src/amd/vulkan/radv_device.c
+++ b/src/amd/vulkan/radv_device.c
@@ -906,16 +906,6 @@ void radv_GetPhysicalDeviceFeatures2(
  features->shaderInt8 = true;
  break;
  }
-    case 
VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_SHADER_ATOMIC_INT64_FEATURES_KHR: {

-    VkPhysicalDeviceShaderAtomicInt64FeaturesKHR *features =
- (VkPhysicalDeviceShaderAtomicInt64FeaturesKHR *)ext;
-    /* TODO: Enable this once the driver supports 64-bit
- * compare atomic operations.
- */
-    features->shaderBufferInt64Atomics = false;
-    features->shaderSharedInt64Atomics = false;
-    break;
-    }
  default:
  break;
  }
diff --git a/src/amd/vulkan/radv_extensions.py 
b/src/amd/vulkan/radv_extensions.py

index 4b12ccc47a0..13fe391e623 100644
--- a/src/amd/vulkan/radv_extensions.py
+++ b/src/amd/vulkan/radv_extensions.py
@@ -81,7 +81,6 @@ EXTENSIONS = [
  Extension('VK_KHR_push_descriptor', 1, True),
  Extension('VK_KHR_relaxed_block_layout', 1, True),
  Extension('VK_KHR_sampler_mirror_clamp_to_edge', 1, True),
-    Extension('VK_KHR_shader_atomic_int64', 1, False),
  Extension('VK_KHR_shader_draw_parameters', 1, True),
  Extension('VK_KHR_shader_float16_int8', 1, True),
  Extension('VK_KHR_storage_buffer_storage_class', 1, True),
diff --git a/src/amd/vulkan/radv_shader.c 
b/src/amd/vulkan/radv_shader.c

index c802abb0e08..a9677094772 100644
--- a/src/amd/vulkan/radv_shader.c
+++ b/src/amd/vulkan/radv_shader.c
@@ -235,7 +235,6 @@ radv_shader_compile_to_nir(struct radv_device 
*device,

  .int8 = true,
  .int16 = true,
  .int64 = true,
-    .int64_atomics = true,
  .multiview = true,
  .physical_storage_buffer_address = true,
  .runtime_descriptor_array = true,


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] Revert "compiler/glsl: handle case where we have multiple users for types"

[Timothy Arceri]

On 18/4/19 4:38 pm, Tapani Pälli wrote:

On 4/18/19 8:50 AM, Timothy Arceri wrote:

On 18/4/19 3:08 pm, Tapani Pälli wrote:



On 4/18/19 8:05 AM, Timothy Arceri wrote:

This reverts commit 624789e3708c87ea2a4c8d2266266b489b421cba.

It caused 400+ piglit tests to crash on radeonsi, I haven't been able
to track down the reason yet.


Could you share the backtrace?


It's more complicated than that. There is some kind of race condition, 
we end up crashing in nir validation as it seems the types are 
released before validation finishes.


I do wonder how that can happen :/ I'm ok with revert;

Acked-by: Tapani Pälli 


I'll hold off pushing for now and try to take a closer look tomorrow. 
Your patch seems correct to me but so did the threading in radeonsi so 
I'm surprised its falling over.






e.g. for ./bin/getuniform-03 -auto -fbo

...
 vec1 32 ssa_404 = deref_array &(*ssa_403)[0] (uniform vec4) /* 
_TextureEnvColor[0] */
error: glsl_type_is_array(parent->type) || 
glsl_type_is_matrix(parent->type) 
(../src/compiler/nir/nir_validate.c:453)


// Tapani

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/3] Revert "radv: add VK_KHR_shader_atomic_int64 but disable it for now"

[Timothy Arceri]

On 18/4/19 4:34 pm, Samuel Pitoiset wrote:


On 4/18/19 8:16 AM, Timothy Arceri wrote:

Meant to add this was tested on LLVM 9.


What do you mean?


I meant to say in the commit messages:

This first patch in this series caused a piglit regression with radeonsi 
NIR on my VEGA64 with LLVM 9.


Well, there is something really strange with RadeonSI NIR then.


??





On 18/4/19 4:15 pm, Timothy Arceri wrote:

This reverts commit 9cf55b022dfa43f8fe3163edeb87a1c25ebf5a16.

This first patch in this series caused a piglit regression with
radeonsi NIR on my VEGA64.

tests/spec/arb_shader_storage_buffer_object/execution/ssbo-atomicAdd-int.shader_test 


---
  src/amd/vulkan/radv_device.c  | 10 --
  src/amd/vulkan/radv_extensions.py |  1 -
  src/amd/vulkan/radv_shader.c  |  1 -
  3 files changed, 12 deletions(-)

diff --git a/src/amd/vulkan/radv_device.c b/src/amd/vulkan/radv_device.c
index 13021a9f2da..1f77dcadb17 100644
--- a/src/amd/vulkan/radv_device.c
+++ b/src/amd/vulkan/radv_device.c
@@ -906,16 +906,6 @@ void radv_GetPhysicalDeviceFeatures2(
  features->shaderInt8 = true;
  break;
  }
-    case 
VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_SHADER_ATOMIC_INT64_FEATURES_KHR: {

-    VkPhysicalDeviceShaderAtomicInt64FeaturesKHR *features =
-    (VkPhysicalDeviceShaderAtomicInt64FeaturesKHR *)ext;
-    /* TODO: Enable this once the driver supports 64-bit
- * compare atomic operations.
- */
-    features->shaderBufferInt64Atomics = false;
-    features->shaderSharedInt64Atomics = false;
-    break;
-    }
  default:
  break;
  }
diff --git a/src/amd/vulkan/radv_extensions.py 
b/src/amd/vulkan/radv_extensions.py

index 4b12ccc47a0..13fe391e623 100644
--- a/src/amd/vulkan/radv_extensions.py
+++ b/src/amd/vulkan/radv_extensions.py
@@ -81,7 +81,6 @@ EXTENSIONS = [
  Extension('VK_KHR_push_descriptor',   1, True),
  Extension('VK_KHR_relaxed_block_layout',  1, True),
  Extension('VK_KHR_sampler_mirror_clamp_to_edge',  1, True),
-    Extension('VK_KHR_shader_atomic_int64',   1, False),
  Extension('VK_KHR_shader_draw_parameters',    1, True),
  Extension('VK_KHR_shader_float16_int8',   1, True),
  Extension('VK_KHR_storage_buffer_storage_class',  1, True),
diff --git a/src/amd/vulkan/radv_shader.c b/src/amd/vulkan/radv_shader.c
index c802abb0e08..a9677094772 100644
--- a/src/amd/vulkan/radv_shader.c
+++ b/src/amd/vulkan/radv_shader.c
@@ -235,7 +235,6 @@ radv_shader_compile_to_nir(struct radv_device 
*device,

  .int8 = true,
  .int16 = true,
  .int64 = true,
-    .int64_atomics = true,
  .multiview = true,
  .physical_storage_buffer_address = true,
  .runtime_descriptor_array = true,


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/3] Revert "radv: add VK_KHR_shader_atomic_int64 but disable it for now"

[Timothy Arceri]

Meant to add this was tested on LLVM 9.

On 18/4/19 4:15 pm, Timothy Arceri wrote:

This reverts commit 9cf55b022dfa43f8fe3163edeb87a1c25ebf5a16.

This first patch in this series caused a piglit regression with
radeonsi NIR on my VEGA64.

tests/spec/arb_shader_storage_buffer_object/execution/ssbo-atomicAdd-int.shader_test
---
  src/amd/vulkan/radv_device.c  | 10 --
  src/amd/vulkan/radv_extensions.py |  1 -
  src/amd/vulkan/radv_shader.c  |  1 -
  3 files changed, 12 deletions(-)

diff --git a/src/amd/vulkan/radv_device.c b/src/amd/vulkan/radv_device.c
index 13021a9f2da..1f77dcadb17 100644
--- a/src/amd/vulkan/radv_device.c
+++ b/src/amd/vulkan/radv_device.c
@@ -906,16 +906,6 @@ void radv_GetPhysicalDeviceFeatures2(
features->shaderInt8 = true;
break;
}
-   case 
VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_SHADER_ATOMIC_INT64_FEATURES_KHR: {
-   VkPhysicalDeviceShaderAtomicInt64FeaturesKHR *features =
-   (VkPhysicalDeviceShaderAtomicInt64FeaturesKHR 
*)ext;
-   /* TODO: Enable this once the driver supports 64-bit
-* compare atomic operations.
-*/
-   features->shaderBufferInt64Atomics = false;
-   features->shaderSharedInt64Atomics = false;
-   break;
-   }
default:
break;
}
diff --git a/src/amd/vulkan/radv_extensions.py 
b/src/amd/vulkan/radv_extensions.py
index 4b12ccc47a0..13fe391e623 100644
--- a/src/amd/vulkan/radv_extensions.py
+++ b/src/amd/vulkan/radv_extensions.py
@@ -81,7 +81,6 @@ EXTENSIONS = [
  Extension('VK_KHR_push_descriptor',   1, True),
  Extension('VK_KHR_relaxed_block_layout',  1, True),
  Extension('VK_KHR_sampler_mirror_clamp_to_edge',  1, True),
-Extension('VK_KHR_shader_atomic_int64',   1, False),
  Extension('VK_KHR_shader_draw_parameters',1, True),
  Extension('VK_KHR_shader_float16_int8',   1, True),
  Extension('VK_KHR_storage_buffer_storage_class',  1, True),
diff --git a/src/amd/vulkan/radv_shader.c b/src/amd/vulkan/radv_shader.c
index c802abb0e08..a9677094772 100644
--- a/src/amd/vulkan/radv_shader.c
+++ b/src/amd/vulkan/radv_shader.c
@@ -235,7 +235,6 @@ radv_shader_compile_to_nir(struct radv_device *device,
.int8 = true,
.int16 = true,
.int64 = true,
-   .int64_atomics = true,
.multiview = true,
.physical_storage_buffer_address = true,
.runtime_descriptor_array = true,


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 1/3] Revert "radv: add VK_KHR_shader_atomic_int64 but disable it for now"

[Timothy Arceri]

This reverts commit 9cf55b022dfa43f8fe3163edeb87a1c25ebf5a16.

This first patch in this series caused a piglit regression with
radeonsi NIR on my VEGA64.

tests/spec/arb_shader_storage_buffer_object/execution/ssbo-atomicAdd-int.shader_test
---
 src/amd/vulkan/radv_device.c  | 10 --
 src/amd/vulkan/radv_extensions.py |  1 -
 src/amd/vulkan/radv_shader.c  |  1 -
 3 files changed, 12 deletions(-)

diff --git a/src/amd/vulkan/radv_device.c b/src/amd/vulkan/radv_device.c
index 13021a9f2da..1f77dcadb17 100644
--- a/src/amd/vulkan/radv_device.c
+++ b/src/amd/vulkan/radv_device.c
@@ -906,16 +906,6 @@ void radv_GetPhysicalDeviceFeatures2(
features->shaderInt8 = true;
break;
}
-   case 
VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_SHADER_ATOMIC_INT64_FEATURES_KHR: {
-   VkPhysicalDeviceShaderAtomicInt64FeaturesKHR *features =
-   (VkPhysicalDeviceShaderAtomicInt64FeaturesKHR 
*)ext;
-   /* TODO: Enable this once the driver supports 64-bit
-* compare atomic operations.
-*/
-   features->shaderBufferInt64Atomics = false;
-   features->shaderSharedInt64Atomics = false;
-   break;
-   }
default:
break;
}
diff --git a/src/amd/vulkan/radv_extensions.py 
b/src/amd/vulkan/radv_extensions.py
index 4b12ccc47a0..13fe391e623 100644
--- a/src/amd/vulkan/radv_extensions.py
+++ b/src/amd/vulkan/radv_extensions.py
@@ -81,7 +81,6 @@ EXTENSIONS = [
 Extension('VK_KHR_push_descriptor',   1, True),
 Extension('VK_KHR_relaxed_block_layout',  1, True),
 Extension('VK_KHR_sampler_mirror_clamp_to_edge',  1, True),
-Extension('VK_KHR_shader_atomic_int64',   1, False),
 Extension('VK_KHR_shader_draw_parameters',1, True),
 Extension('VK_KHR_shader_float16_int8',   1, True),
 Extension('VK_KHR_storage_buffer_storage_class',  1, True),
diff --git a/src/amd/vulkan/radv_shader.c b/src/amd/vulkan/radv_shader.c
index c802abb0e08..a9677094772 100644
--- a/src/amd/vulkan/radv_shader.c
+++ b/src/amd/vulkan/radv_shader.c
@@ -235,7 +235,6 @@ radv_shader_compile_to_nir(struct radv_device *device,
.int8 = true,
.int16 = true,
.int64 = true,
-   .int64_atomics = true,
.multiview = true,
.physical_storage_buffer_address = true,
.runtime_descriptor_array = true,
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 3/3] Revert "ac/nir: use new LLVM 8 intrinsics for SSBO atomics except cmpswap"

[Timothy Arceri]

This reverts commit 78c551aca1c785470e3c0480e33a072b0b5f8928.

This patch caused a piglit regression with radeonsi NIR on my VEGA64.

tests/spec/arb_shader_storage_buffer_object/execution/ssbo-atomicAdd-int.shader_test
---
 src/amd/common/ac_nir_to_llvm.c | 31 +--
 1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/src/amd/common/ac_nir_to_llvm.c b/src/amd/common/ac_nir_to_llvm.c
index de2a9ed8f67..aa85029aed0 100644
--- a/src/amd/common/ac_nir_to_llvm.c
+++ b/src/amd/common/ac_nir_to_llvm.c
@@ -1648,6 +1648,17 @@ static LLVMValueRef visit_atomic_ssbo(struct 
ac_nir_context *ctx,
LLVMValueRef params[6];
int arg_count = 0;
 
+   if (instr->intrinsic == nir_intrinsic_ssbo_atomic_comp_swap) {
+   params[arg_count++] = ac_llvm_extract_elem(>ac, 
get_src(ctx, instr->src[3]), 0);
+   }
+   params[arg_count++] = ac_llvm_extract_elem(>ac, get_src(ctx, 
instr->src[2]), 0);
+   params[arg_count++] = ctx->abi->load_ssbo(ctx->abi,
+get_src(ctx, instr->src[0]),
+true);
+   params[arg_count++] = ctx->ac.i32_0; /* vindex */
+   params[arg_count++] = get_src(ctx, instr->src[1]);  /* voffset */
+   params[arg_count++] = ctx->ac.i1false;  /* slc */
+
switch (instr->intrinsic) {
case nir_intrinsic_ssbo_atomic_add:
op = "add";
@@ -1683,27 +1694,11 @@ static LLVMValueRef visit_atomic_ssbo(struct 
ac_nir_context *ctx,
abort();
}
 
-   if (instr->intrinsic == nir_intrinsic_ssbo_atomic_comp_swap) {
-   params[arg_count++] = ac_llvm_extract_elem(>ac, 
get_src(ctx, instr->src[3]), 0);
-   }
-   params[arg_count++] = ac_llvm_extract_elem(>ac, get_src(ctx, 
instr->src[2]), 0);
-   params[arg_count++] = ctx->abi->load_ssbo(ctx->abi,
- get_src(ctx, instr->src[0]),
- true);
-
-   if (HAVE_LLVM >= 0x800 &&
+   if (HAVE_LLVM >= 0x900 &&
instr->intrinsic != nir_intrinsic_ssbo_atomic_comp_swap) {
-   params[arg_count++] = get_src(ctx, instr->src[1]); /* voffset */
-   params[arg_count++] = ctx->ac.i32_0; /* soffset */
-   params[arg_count++] = ctx->ac.i32_0; /* slc */
-
snprintf(name, sizeof(name),
-"llvm.amdgcn.raw.buffer.atomic.%s.i32", op);
+"llvm.amdgcn.buffer.atomic.%s.i32", op);
} else {
-   params[arg_count++] = ctx->ac.i32_0; /* vindex */
-   params[arg_count++] = get_src(ctx, instr->src[1]); /* voffset */
-   params[arg_count++] = ctx->ac.i1false; /* slc */
-
snprintf(name, sizeof(name),
 "llvm.amdgcn.buffer.atomic.%s", op);
}
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 2/3] Revert "ac/nir: add 64-bit SSBO atomic operations support"

[Timothy Arceri]

This reverts commit d118e382dd037696ff904e230b11028abf214e80.

This first patch in this series caused a piglit regression with
radeonsi NIR on my VEGA64.

tests/spec/arb_shader_storage_buffer_object/execution/ssbo-atomicAdd-int.shader_test
---
 src/amd/common/ac_nir_to_llvm.c | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/src/amd/common/ac_nir_to_llvm.c b/src/amd/common/ac_nir_to_llvm.c
index 3890aebc982..de2a9ed8f67 100644
--- a/src/amd/common/ac_nir_to_llvm.c
+++ b/src/amd/common/ac_nir_to_llvm.c
@@ -1643,9 +1643,8 @@ static void visit_store_ssbo(struct ac_nir_context *ctx,
 static LLVMValueRef visit_atomic_ssbo(struct ac_nir_context *ctx,
   const nir_intrinsic_instr *instr)
 {
-   LLVMTypeRef return_type = LLVMTypeOf(get_src(ctx, instr->src[2]));
const char *op;
-   char name[64], type[8];
+   char name[64];
LLVMValueRef params[6];
int arg_count = 0;
 
@@ -1698,21 +1697,18 @@ static LLVMValueRef visit_atomic_ssbo(struct 
ac_nir_context *ctx,
params[arg_count++] = ctx->ac.i32_0; /* soffset */
params[arg_count++] = ctx->ac.i32_0; /* slc */
 
-   ac_build_type_name_for_intr(return_type, type, sizeof(type));
snprintf(name, sizeof(name),
-"llvm.amdgcn.raw.buffer.atomic.%s.%s", op, type);
+"llvm.amdgcn.raw.buffer.atomic.%s.i32", op);
} else {
params[arg_count++] = ctx->ac.i32_0; /* vindex */
params[arg_count++] = get_src(ctx, instr->src[1]); /* voffset */
params[arg_count++] = ctx->ac.i1false; /* slc */
 
-   assert(return_type == ctx->ac.i32);
snprintf(name, sizeof(name),
 "llvm.amdgcn.buffer.atomic.%s", op);
}
 
-   return ac_build_intrinsic(>ac, name, return_type, params,
- arg_count, 0);
+   return ac_build_intrinsic(>ac, name, ctx->ac.i32, params, 
arg_count, 0);
 }
 
 static LLVMValueRef visit_load_buffer(struct ac_nir_context *ctx,
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] Revert "compiler/glsl: handle case where we have multiple users for types"

[Timothy Arceri]

On 18/4/19 3:08 pm, Tapani Pälli wrote:



On 4/18/19 8:05 AM, Timothy Arceri wrote:

This reverts commit 624789e3708c87ea2a4c8d2266266b489b421cba.

It caused 400+ piglit tests to crash on radeonsi, I haven't been able
to track down the reason yet.


Could you share the backtrace?


It's more complicated than that. There is some kind of race condition, 
we end up crashing in nir validation as it seems the types are released 
before validation finishes.


e.g. for ./bin/getuniform-03 -auto -fbo

...
	vec1 32 ssa_404 = deref_array &(*ssa_403)[0] (uniform vec4) /* 
_TextureEnvColor[0] */
error: glsl_type_is_array(parent->type) || 
glsl_type_is_matrix(parent->type) (../src/compiler/nir/nir_validate.c:453)

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] Revert "compiler/glsl: handle case where we have multiple users for types"

[Timothy Arceri]

This reverts commit 624789e3708c87ea2a4c8d2266266b489b421cba.

It caused 400+ piglit tests to crash on radeonsi, I haven't been able
to track down the reason yet.
---
 src/amd/vulkan/radv_device.c |  3 ---
 src/compiler/glsl/glsl_parser_extras.cpp | 20 ++-
 src/compiler/glsl/glsl_parser_extras.h   |  2 --
 src/compiler/glsl/standalone.cpp |  3 +--
 src/compiler/glsl_types.cpp  | 32 
 src/compiler/glsl_types.h| 10 +++-
 src/intel/vulkan/anv_device.c|  3 ---
 src/mesa/main/context.c  |  4 ---
 8 files changed, 11 insertions(+), 66 deletions(-)

diff --git a/src/amd/vulkan/radv_device.c b/src/amd/vulkan/radv_device.c
index 96c6543141e..13021a9f2da 100644
--- a/src/amd/vulkan/radv_device.c
+++ b/src/amd/vulkan/radv_device.c
@@ -48,7 +48,6 @@
 #include "util/build_id.h"
 #include "util/debug.h"
 #include "util/mesa-sha1.h"
-#include "compiler/glsl_types.h"
 
 static int
 radv_device_get_cache_uuid(enum radeon_family family, void *uuid)
@@ -583,7 +582,6 @@ VkResult radv_CreateInstance(
}
 
_mesa_locale_init();
-   glsl_type_singleton_init_or_ref();
 
VG(VALGRIND_CREATE_MEMPOOL(instance, 0, false));
 
@@ -609,7 +607,6 @@ void radv_DestroyInstance(
 
VG(VALGRIND_DESTROY_MEMPOOL(instance));
 
-   glsl_type_singleton_decref();
_mesa_locale_fini();
 
vk_debug_report_instance_destroy(>debug_report_callbacks);
diff --git a/src/compiler/glsl/glsl_parser_extras.cpp 
b/src/compiler/glsl/glsl_parser_extras.cpp
index b30c638cdc9..0ffad2d25a0 100644
--- a/src/compiler/glsl/glsl_parser_extras.cpp
+++ b/src/compiler/glsl/glsl_parser_extras.cpp
@@ -2324,24 +2324,6 @@ do_common_optimization(exec_list *ir, bool linked,
 
 extern "C" {
 
-/**
- * To be called at GL context ctor.
- */
-void
-_mesa_init_shader_compiler_types(void)
-{
-   glsl_type_singleton_init_or_ref();
-}
-
-/**
- * To be called at GL context dtor.
- */
-void
-_mesa_destroy_shader_compiler_types(void)
-{
-   glsl_type_singleton_decref();
-}
-
 /**
  * To be called at GL teardown time, this frees compiler datastructures.
  *
@@ -2353,6 +2335,8 @@ void
 _mesa_destroy_shader_compiler(void)
 {
_mesa_destroy_shader_compiler_caches();
+
+   _mesa_glsl_release_types();
 }
 
 /**
diff --git a/src/compiler/glsl/glsl_parser_extras.h 
b/src/compiler/glsl/glsl_parser_extras.h
index f92d2160aac..8646ba6cadd 100644
--- a/src/compiler/glsl/glsl_parser_extras.h
+++ b/src/compiler/glsl/glsl_parser_extras.h
@@ -1010,8 +1010,6 @@ extern int glcpp_preprocess(void *ctx, const char 
**shader, char **info_log,
 struct _mesa_glsl_parse_state *state,
 struct gl_context *gl_ctx);
 
-extern void _mesa_init_shader_compiler_types(void);
-extern void _mesa_destroy_shader_compiler_types(void);
 extern void _mesa_destroy_shader_compiler(void);
 extern void _mesa_destroy_shader_compiler_caches(void);
 
diff --git a/src/compiler/glsl/standalone.cpp b/src/compiler/glsl/standalone.cpp
index 7b3d358ca96..942b9ee4986 100644
--- a/src/compiler/glsl/standalone.cpp
+++ b/src/compiler/glsl/standalone.cpp
@@ -132,7 +132,6 @@ static void
 initialize_context(struct gl_context *ctx, gl_api api)
 {
initialize_context_to_defaults(ctx, api);
-   glsl_type_singleton_init_or_ref();
 
/* The standalone compiler needs to claim support for almost
 * everything in order to compile the built-in functions.
@@ -618,6 +617,6 @@ standalone_compiler_cleanup(struct gl_shader_program 
*whole_program)
delete whole_program->FragDataIndexBindings;
 
ralloc_free(whole_program);
-   glsl_type_singleton_decref();
+   _mesa_glsl_release_types();
_mesa_glsl_release_builtin_functions();
 }
diff --git a/src/compiler/glsl_types.cpp b/src/compiler/glsl_types.cpp
index 9938b3df450..66241b34281 100644
--- a/src/compiler/glsl_types.cpp
+++ b/src/compiler/glsl_types.cpp
@@ -37,12 +37,6 @@ hash_table *glsl_type::interface_types = NULL;
 hash_table *glsl_type::function_types = NULL;
 hash_table *glsl_type::subroutine_types = NULL;
 
-/* There might be multiple users for types (e.g. application using OpenGL
- * and Vulkan simultanously or app using multiple Vulkan instances). Counter
- * is used to make sure we don't release the types if a user is still present.
- */
-static uint32_t glsl_type_users = 0;
-
 glsl_type::glsl_type(GLenum gl_type,
  glsl_base_type base_type, unsigned vector_elements,
  unsigned matrix_columns, const char *name,
@@ -475,26 +469,12 @@ hash_free_type_function(struct hash_entry *entry)
 }
 
 void
-glsl_type_singleton_init_or_ref()
+_mesa_glsl_release_types(void)
 {
-   mtx_lock(_type::hash_mutex);
-   glsl_type_users++;
-   mtx_unlock(_type::hash_mutex);
-}
-
-void
-glsl_type_singleton_decref()
-{
-   mtx_lock(_type::hash_mutex);
-
-   assert(glsl_type_users > 0);
-
-   /* Do not release glsl_types if they are still used. */
- 

[Mesa-dev] [PATCH] radeonsi/nir: fix scanning of bindless images

[Timothy Arceri]

Fixes: d62d434fe920 ("ac/nir_to_llvm: add image bindless support")
---
 src/gallium/drivers/radeonsi/si_shader_nir.c | 75 ++--
 1 file changed, 37 insertions(+), 38 deletions(-)

diff --git a/src/gallium/drivers/radeonsi/si_shader_nir.c 
b/src/gallium/drivers/radeonsi/si_shader_nir.c
index 938b0efcb76..5a925f19e09 100644
--- a/src/gallium/drivers/radeonsi/si_shader_nir.c
+++ b/src/gallium/drivers/radeonsi/si_shader_nir.c
@@ -181,39 +181,48 @@ static void scan_instruction(const struct nir_shader *nir,
case nir_intrinsic_load_tess_level_outer:
info->reads_tess_factors = true;
break;
-   case nir_intrinsic_image_deref_load: {
-   nir_variable *var = intrinsic_get_var(intr);
-   if (var->data.bindless) {
-   info->uses_bindless_images = true;
+   case nir_intrinsic_bindless_image_load:
+   info->uses_bindless_images = true;
 
-   if (glsl_get_sampler_dim(var->type) == 
GLSL_SAMPLER_DIM_BUF)
-   info->uses_bindless_buffer_load = true;
-   else
-   info->uses_bindless_image_load = true;
-   }
+   if (nir_intrinsic_image_dim(intr) == 
GLSL_SAMPLER_DIM_BUF)
+   info->uses_bindless_buffer_load = true;
+   else
+   info->uses_bindless_image_load = true;
break;
-   }
-   case nir_intrinsic_image_deref_size:
-   case nir_intrinsic_image_deref_samples: {
-   nir_variable *var = intrinsic_get_var(intr);
-   if (var->data.bindless)
-   info->uses_bindless_images = true;
+   case nir_intrinsic_bindless_image_size:
+   case nir_intrinsic_bindless_image_samples:
+   info->uses_bindless_images = true;
break;
-   }
-   case nir_intrinsic_image_deref_store: {
-   const nir_deref_instr *image_deref = 
nir_instr_as_deref(intr->src[0].ssa->parent_instr);
-   nir_variable *var = intrinsic_get_var(intr);
-   if (var->data.bindless) {
-   info->uses_bindless_images = true;
+   case nir_intrinsic_bindless_image_store:
+   info->uses_bindless_images = true;
+
+   if (nir_intrinsic_image_dim(intr) == 
GLSL_SAMPLER_DIM_BUF)
+   info->uses_bindless_buffer_store = true;
+   else
+   info->uses_bindless_image_store = true;
+
+   info->writes_memory = true;
+   break;
+   case nir_intrinsic_image_deref_store:
+   info->writes_memory = true;
+   break;
+   case nir_intrinsic_bindless_image_atomic_add:
+   case nir_intrinsic_bindless_image_atomic_min:
+   case nir_intrinsic_bindless_image_atomic_max:
+   case nir_intrinsic_bindless_image_atomic_and:
+   case nir_intrinsic_bindless_image_atomic_or:
+   case nir_intrinsic_bindless_image_atomic_xor:
+   case nir_intrinsic_bindless_image_atomic_exchange:
+   case nir_intrinsic_bindless_image_atomic_comp_swap:
+   info->uses_bindless_images = true;
+
+   if (nir_intrinsic_image_dim(intr) == 
GLSL_SAMPLER_DIM_BUF)
+   info->uses_bindless_buffer_atomic = true;
+   else
+   info->uses_bindless_image_atomic = true;
 
-   if (glsl_get_sampler_dim(image_deref->type) == 
GLSL_SAMPLER_DIM_BUF)
-   info->uses_bindless_buffer_store = true;
-   else
-   info->uses_bindless_image_store = true;
-   }
info->writes_memory = true;
break;
-   }
case nir_intrinsic_image_deref_atomic_add:
case nir_intrinsic_image_deref_atomic_min:
case nir_intrinsic_image_deref_atomic_max:
@@ -221,19 +230,9 @@ static void scan_instruction(const struct nir_shader *nir,
case nir_intrinsic_image_deref_atomic_or:
case nir_intrinsic_image_deref_atomic_xor:
case nir_intrinsic_image_deref_atomic_exchange:
-   case nir_intrinsic_image_deref_atomic_comp_swap: {
-   nir_variable *var = intrinsic_get_var(intr);
-   if (var->data.bindless) {

Re: [Mesa-dev] [PATCH] nir: optimize gl_SampleMaskIn to gl_HelperInvocation for radeonsi when possible

[Timothy Arceri]

On 16/4/19 1:14 pm, Marek Olšák wrote:



On Mon, Apr 15, 2019, 10:41 PM Timothy Arceri <mailto:tarc...@itsqueeze.com>> wrote:




On 16/4/19 7:03 am, Marek Olšák wrote:
 > ping
 >
 > On Tue, Apr 9, 2019 at 10:03 PM Marek Olšák mailto:mar...@gmail.com>
 > <mailto:mar...@gmail.com <mailto:mar...@gmail.com>>> wrote:
 >
 >     From: Marek Olšák mailto:marek.ol...@amd.com> <mailto:marek.ol...@amd.com
<mailto:marek.ol...@amd.com>>>
 >
 >     ---
 >       src/compiler/nir/nir.h                    |  8 +
 >       src/compiler/nir/nir_opt_intrinsics.c     | 40
+--
 >       src/gallium/drivers/radeonsi/si_get.c     |  1 +
 >       src/mesa/state_tracker/st_glsl_to_nir.cpp |  1 +
 >       4 files changed, 48 insertions(+), 2 deletions(-)
 >
 >     diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
 >     index 09950bf3398..d3874705235 100644
 >     --- a/src/compiler/nir/nir.h
 >     +++ b/src/compiler/nir/nir.h
 >     @@ -2283,20 +2283,28 @@ typedef struct
nir_shader_compiler_options {
 >           *
 >           * This depends on some possibly hw implementation details,
 >     which may
 >           * not be true for all hw.  In particular that the FS is
only
 >     executed
 >           * for covered samples or for helper invocations.  So,
do not
 >     blindly
 >           * enable this option.
 >           *
 >           * Note: See also issue #22 in ARB_shader_image_load_store
 >           */
 >          bool lower_helper_invocation;
 >
 >     +   /**
 >     +    * Convert gl_SampleMaskIn to gl_HelperInvocation as follows:
 >     +    *
 >     +    *   gl_SampleMaskIn == 0 ---> gl_HelperInvocation
 >     +    *   gl_SampleMaskIn != 0 ---> !gl_HelperInvocation
 >     +    */
 >     +   bool optimize_sample_mask_in;
 >     +
 >          bool lower_cs_local_index_from_id;
 >          bool lower_cs_local_id_from_index;
 >
 >          bool lower_device_index_to_zero;
 >
 >          /* Set if nir_lower_wpos_ytransform() should also invert
 >     gl_PointCoord. */
 >          bool lower_wpos_pntc;
 >
 >          bool lower_hadd;
 >          bool lower_add_sat;
 >     diff --git a/src/compiler/nir/nir_opt_intrinsics.c
 >     b/src/compiler/nir/nir_opt_intrinsics.c
 >     index 7b054faa204..2f9e4e466c9 100644
 >     --- a/src/compiler/nir/nir_opt_intrinsics.c
 >     +++ b/src/compiler/nir/nir_opt_intrinsics.c
 >     @@ -22,21 +22,22 @@
 >        */
 >
 >       #include "nir.h"
 >       #include "nir_builder.h"
 >
 >       /**
 >        * \file nir_opt_intrinsics.c
 >        */
 >
 >       static bool
 >     -opt_intrinsics_impl(nir_function_impl *impl)
 >     +opt_intrinsics_impl(nir_function_impl *impl,
 >     +                    const struct nir_shader_compiler_options
*options)
 >       {
 >          nir_builder b;
 >          nir_builder_init(, impl);
 >          bool progress = false;
 >
 >          nir_foreach_block(block, impl) {
 >             nir_foreach_instr_safe(instr, block) {
 >                if (instr->type != nir_instr_type_intrinsic)
 >                   continue;
 >
 >     @@ -48,20 +49,55 @@ opt_intrinsics_impl(nir_function_impl *impl)
 >                case nir_intrinsic_vote_any:
 >                case nir_intrinsic_vote_all:
 >                   if (nir_src_is_const(intrin->src[0]))
 >                      replacement = nir_ssa_for_src(,
intrin->src[0], 1);
 >                   break;
 >                case nir_intrinsic_vote_feq:
 >                case nir_intrinsic_vote_ieq:
 >                   if (nir_src_is_const(intrin->src[0]))
 >                      replacement = nir_imm_true();
 >                   break;
 >     +         case nir_intrinsic_load_sample_mask_in:
 >     +            /* Transform:
 >     +             *   gl_SampleMaskIn == 0 ---> gl_HelperInvocation
 >     +             *   gl_SampleMaskIn != 0 ---> !gl_HelperInvocation
 >     +             */
 >     +            if (!options->optimize_sample_mask_in)
 >     +               continue;
 >     +
 >     +            nir_foreach_use_safe(use_src, >dest.ssa) {
 >     +               if (use_src->parent_ins

Re: [Mesa-dev] [PATCH] nir: optimize gl_SampleMaskIn to gl_HelperInvocation for radeonsi when possible

[Timothy Arceri]

On 16/4/19 7:03 am, Marek Olšák wrote:

ping

On Tue, Apr 9, 2019 at 10:03 PM Marek Olšák > wrote:


From: Marek Olšák mailto:marek.ol...@amd.com>>

---
  src/compiler/nir/nir.h                    |  8 +
  src/compiler/nir/nir_opt_intrinsics.c     | 40 +--
  src/gallium/drivers/radeonsi/si_get.c     |  1 +
  src/mesa/state_tracker/st_glsl_to_nir.cpp |  1 +
  4 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
index 09950bf3398..d3874705235 100644
--- a/src/compiler/nir/nir.h
+++ b/src/compiler/nir/nir.h
@@ -2283,20 +2283,28 @@ typedef struct nir_shader_compiler_options {
      *
      * This depends on some possibly hw implementation details,
which may
      * not be true for all hw.  In particular that the FS is only
executed
      * for covered samples or for helper invocations.  So, do not
blindly
      * enable this option.
      *
      * Note: See also issue #22 in ARB_shader_image_load_store
      */
     bool lower_helper_invocation;

+   /**
+    * Convert gl_SampleMaskIn to gl_HelperInvocation as follows:
+    *
+    *   gl_SampleMaskIn == 0 ---> gl_HelperInvocation
+    *   gl_SampleMaskIn != 0 ---> !gl_HelperInvocation
+    */
+   bool optimize_sample_mask_in;
+
     bool lower_cs_local_index_from_id;
     bool lower_cs_local_id_from_index;

     bool lower_device_index_to_zero;

     /* Set if nir_lower_wpos_ytransform() should also invert
gl_PointCoord. */
     bool lower_wpos_pntc;

     bool lower_hadd;
     bool lower_add_sat;
diff --git a/src/compiler/nir/nir_opt_intrinsics.c
b/src/compiler/nir/nir_opt_intrinsics.c
index 7b054faa204..2f9e4e466c9 100644
--- a/src/compiler/nir/nir_opt_intrinsics.c
+++ b/src/compiler/nir/nir_opt_intrinsics.c
@@ -22,21 +22,22 @@
   */

  #include "nir.h"
  #include "nir_builder.h"

  /**
   * \file nir_opt_intrinsics.c
   */

  static bool
-opt_intrinsics_impl(nir_function_impl *impl)
+opt_intrinsics_impl(nir_function_impl *impl,
+                    const struct nir_shader_compiler_options *options)
  {
     nir_builder b;
     nir_builder_init(, impl);
     bool progress = false;

     nir_foreach_block(block, impl) {
        nir_foreach_instr_safe(instr, block) {
           if (instr->type != nir_instr_type_intrinsic)
              continue;

@@ -48,20 +49,55 @@ opt_intrinsics_impl(nir_function_impl *impl)
           case nir_intrinsic_vote_any:
           case nir_intrinsic_vote_all:
              if (nir_src_is_const(intrin->src[0]))
                 replacement = nir_ssa_for_src(, intrin->src[0], 1);
              break;
           case nir_intrinsic_vote_feq:
           case nir_intrinsic_vote_ieq:
              if (nir_src_is_const(intrin->src[0]))
                 replacement = nir_imm_true();
              break;
+         case nir_intrinsic_load_sample_mask_in:
+            /* Transform:
+             *   gl_SampleMaskIn == 0 ---> gl_HelperInvocation
+             *   gl_SampleMaskIn != 0 ---> !gl_HelperInvocation
+             */
+            if (!options->optimize_sample_mask_in)
+               continue;
+
+            nir_foreach_use_safe(use_src, >dest.ssa) {
+               if (use_src->parent_instr->type == nir_instr_type_alu) {
+                  nir_alu_instr *alu =
nir_instr_as_alu(use_src->parent_instr);
+
+                  if (alu->op == nir_op_ieq ||
+                      alu->op == nir_op_ine) {
+                     /* Check for 0 in either operand. */
+                     nir_const_value *const_val =
+                         nir_src_as_const_value(alu->src[0].src);
+                     if (!const_val)
+                        const_val =
nir_src_as_const_value(alu->src[1].src);
+                     if (!const_val || const_val->i32[0] != 0)
+                        continue;
+
+                     nir_ssa_def *new_expr =
nir_load_helper_invocation(, 1);
+
+                     if (alu->op == nir_op_ine32)


How can this be nir_op_ine32 when the outer if condition is:

alu->op == nir_op_ieq || alu->op == nir_op_ine

??


+                        new_expr = nir_inot(, new_expr);
+
+                     nir_ssa_def_rewrite_uses(>dest.dest.ssa,
+ 
nir_src_for_ssa(new_expr));

+                     nir_instr_remove(>instr);
+                     continue;
+                  }
+               }
+            }
+            continue;
           default:
              break;
           }

           if (!replacement)
  

Re: [Mesa-dev] [PATCH] nir: fix packing components with arrays

[Timothy Arceri]

On 15/4/19 5:53 pm, Samuel Pitoiset wrote:

Shouldn't we also apply that fix in gather_varying_component_info() ?



No because we don't enter that code path for arrays because we don't try 
to pack arrays into other slots/components. But we do pack scalar 
components with "unmovable" arrays when there is space. Hopefully that 
makes sense.



On 4/15/19 7:00 AM, Timothy Arceri wrote:

When gathering info for unmovable types we need to handle arrays.
While we dont support packing/moving arrays we do support packing
scalar components with these arrays.

Fixes piglit:
tests/spec/arb_enhanced_layouts/execution/component-layout/vs-fs-array-interleave-range.shader_test 



Fixes: 5eb17506e159 ("nir: do not pack varying with different types")
---
  src/compiler/nir/nir_linking_helpers.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/compiler/nir/nir_linking_helpers.c 
b/src/compiler/nir/nir_linking_helpers.c

index 7594728e25e..f4494c78f98 100644
--- a/src/compiler/nir/nir_linking_helpers.c
+++ b/src/compiler/nir/nir_linking_helpers.c
@@ -309,7 +309,8 @@ get_unmoveable_components_masks(struct exec_list 
*var_list,

  comps[location + i].interp_type =
 get_interp_type(var, type, default_to_smooth_interp);
  comps[location + i].interp_loc = get_interp_loc(var);
-    comps[location + i].is_32bit = glsl_type_is_32bit(type);
+    comps[location + i].is_32bit =
+   glsl_type_is_32bit(glsl_without_array(type));
   }
    }
 }

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] nir: fix packing components with arrays

[Timothy Arceri]

When gathering info for unmovable types we need to handle arrays.
While we dont support packing/moving arrays we do support packing
scalar components with these arrays.

Fixes piglit:
tests/spec/arb_enhanced_layouts/execution/component-layout/vs-fs-array-interleave-range.shader_test

Fixes: 5eb17506e159 ("nir: do not pack varying with different types")
---
 src/compiler/nir/nir_linking_helpers.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/compiler/nir/nir_linking_helpers.c 
b/src/compiler/nir/nir_linking_helpers.c
index 7594728e25e..f4494c78f98 100644
--- a/src/compiler/nir/nir_linking_helpers.c
+++ b/src/compiler/nir/nir_linking_helpers.c
@@ -309,7 +309,8 @@ get_unmoveable_components_masks(struct exec_list *var_list,
 comps[location + i].interp_type =
get_interp_type(var, type, default_to_smooth_interp);
 comps[location + i].interp_loc = get_interp_loc(var);
-comps[location + i].is_32bit = glsl_type_is_32bit(type);
+comps[location + i].is_32bit =
+   glsl_type_is_32bit(glsl_without_array(type));
  }
   }
}
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] ac: fix possibly incorrect bindless atomic code in visit_image_atomic

[Timothy Arceri]

Thanks I didn't see this and sent the same patch. Please add the 
following to the commit message:


Coverity: CID 1444664

Fixes: d62d434fe920 ("ac/nir_to_llvm: add image bindless support")

With those:

Reviewed-by: Timothy Arceri 

On 13/4/19 1:40 am, Marek Olšák wrote:

From: Marek Olšák 

---
  src/amd/common/ac_nir_to_llvm.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/amd/common/ac_nir_to_llvm.c b/src/amd/common/ac_nir_to_llvm.c
index 55c64e2aacb..afdd9318fff 100644
--- a/src/amd/common/ac_nir_to_llvm.c
+++ b/src/amd/common/ac_nir_to_llvm.c
@@ -2543,25 +2543,25 @@ static LLVMValueRef visit_image_atomic(struct 
ac_nir_context *ctx,
int param_count = 0;
  
  	bool cmpswap = instr->intrinsic == nir_intrinsic_image_deref_atomic_comp_swap ||

   instr->intrinsic == 
nir_intrinsic_bindless_image_atomic_comp_swap;
const char *atomic_name;
char intrinsic_name[64];
enum ac_atomic_op atomic_subop;
MAYBE_UNUSED int length;
  
  	enum glsl_sampler_dim dim;

-   bool is_unsigned;
+   bool is_unsigned = false;
bool is_array;
if (bindless) {
-   if (instr->intrinsic == nir_intrinsic_image_atomic_min ||
-   instr->intrinsic == nir_intrinsic_image_atomic_max) {
+   if (instr->intrinsic == nir_intrinsic_bindless_image_atomic_min 
||
+   instr->intrinsic == 
nir_intrinsic_bindless_image_atomic_max) {
const GLenum format = nir_intrinsic_format(instr);
assert(format == GL_R32UI || format == GL_R32I);
is_unsigned = format == GL_R32UI;
}
dim = nir_intrinsic_image_dim(instr);
is_array = nir_intrinsic_image_array(instr);
} else {
const struct glsl_type *type = get_image_deref(instr)->type;
is_unsigned = glsl_get_sampler_result_type(type) == 
GLSL_TYPE_UINT;
dim = glsl_get_sampler_dim(type);


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] ac/nir_to_llvm: use correct intrinsic type for bindless atomic_{min, max}

[Timothy Arceri]

Coverity: CID 1444664

Fixes: d62d434fe920 ("ac/nir_to_llvm: add image bindless support")
---
 src/amd/common/ac_nir_to_llvm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/amd/common/ac_nir_to_llvm.c b/src/amd/common/ac_nir_to_llvm.c
index 61b33c74e6c..8266e7e9f68 100644
--- a/src/amd/common/ac_nir_to_llvm.c
+++ b/src/amd/common/ac_nir_to_llvm.c
@@ -2549,8 +2549,8 @@ static LLVMValueRef visit_image_atomic(struct 
ac_nir_context *ctx,
bool is_unsigned;
bool is_array;
if (bindless) {
-   if (instr->intrinsic == nir_intrinsic_image_atomic_min ||
-   instr->intrinsic == nir_intrinsic_image_atomic_max) {
+   if (instr->intrinsic == nir_intrinsic_bindless_image_atomic_min 
||
+   instr->intrinsic == 
nir_intrinsic_bindless_image_atomic_max) {
const GLenum format = nir_intrinsic_format(instr);
assert(format == GL_R32UI || format == GL_R32I);
is_unsigned = format == GL_R32UI;
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] Mesa compatibility range

[Timothy Arceri]

Not really answering your question directly but Desktop chips have had 
2.1 support for the past 12+ years. IMO trying to support systems older 
than that is going to be more trouble than its worth, llvmpipe is 
unlikely to give you very good performance on a system that old.


On 11/4/19 1:37 am, Jonathan Jackson wrote:

Hi all,

I am currently building an application using VTK 8.1 which requires 
OpenGL 2.1, or more recent,


Support. Because I want to be able to run my application on as many 
systems as possible, I am


Contemplating using a software renderer for cases where the system does 
not support OpenGL 2.1.


As Mesa is already offered on linux, this is mostly a Windows question.

1.My question is: Is it reasonable to think that llvmpipe (or other 
drivers) will grand me a much wider


array of supported system if I use that implementation over the original 
Windows one?


It would mean that systems that have no support for OpenGL 2.1 would 
benefit from using Mesa over


Windows’ OpenGL and that Mesa is usable on much older/weaker systems.

2.Another way to ask the question is: Is there a way of knowing which 
specs a system is required to have


so it can support Mesa’s implementation of all of OpenGL 2.1? (is it 
common that a system(high or low end)


will support all of Mesa’s equivalents of OpenGL 2.1 features).

Thank you for your support.

Jonathan Jackson


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 10/10] ac/nir: remove some useless integer casts for ALU operations

[Timothy Arceri]

Before I attempt to review, have you run piglit on this series with 
radeonsi nir?


On 11/4/19 1:16 am, Samuel Pitoiset wrote:

Sources are always casted to integers.

Signed-off-by: Samuel Pitoiset 
---
  src/amd/common/ac_nir_to_llvm.c | 16 
  1 file changed, 16 deletions(-)

diff --git a/src/amd/common/ac_nir_to_llvm.c b/src/amd/common/ac_nir_to_llvm.c
index 3dfcbacdef4..f1c082c1130 100644
--- a/src/amd/common/ac_nir_to_llvm.c
+++ b/src/amd/common/ac_nir_to_llvm.c
@@ -874,13 +874,11 @@ static void visit_alu(struct ac_nir_context *ctx, const 
nir_alu_instr *instr)
case nir_op_i2f16:
case nir_op_i2f32:
case nir_op_i2f64:
-   src[0] = ac_to_integer(>ac, src[0]);
result = LLVMBuildSIToFP(ctx->ac.builder, src[0], 
ac_to_float_type(>ac, def_type), "");
break;
case nir_op_u2f16:
case nir_op_u2f32:
case nir_op_u2f64:
-   src[0] = ac_to_integer(>ac, src[0]);
result = LLVMBuildUIToFP(ctx->ac.builder, src[0], 
ac_to_float_type(>ac, def_type), "");
break;
case nir_op_f2f16_rtz:
@@ -905,7 +903,6 @@ static void visit_alu(struct ac_nir_context *ctx, const 
nir_alu_instr *instr)
case nir_op_u2u16:
case nir_op_u2u32:
case nir_op_u2u64:
-   src[0] = ac_to_integer(>ac, src[0]);
if (ac_get_elem_bits(>ac, LLVMTypeOf(src[0])) < 
ac_get_elem_bits(>ac, def_type))
result = LLVMBuildZExt(ctx->ac.builder, src[0], def_type, 
"");
else
@@ -915,7 +912,6 @@ static void visit_alu(struct ac_nir_context *ctx, const 
nir_alu_instr *instr)
case nir_op_i2i16:
case nir_op_i2i32:
case nir_op_i2i64:
-   src[0] = ac_to_integer(>ac, src[0]);
if (ac_get_elem_bits(>ac, LLVMTypeOf(src[0])) < 
ac_get_elem_bits(>ac, def_type))
result = LLVMBuildSExt(ctx->ac.builder, src[0], def_type, 
"");
else
@@ -925,25 +921,18 @@ static void visit_alu(struct ac_nir_context *ctx, const 
nir_alu_instr *instr)
result = emit_bcsel(>ac, src[0], src[1], src[2]);
break;
case nir_op_find_lsb:
-   src[0] = ac_to_integer(>ac, src[0]);
result = ac_find_lsb(>ac, ctx->ac.i32, src[0]);
break;
case nir_op_ufind_msb:
-   src[0] = ac_to_integer(>ac, src[0]);
result = ac_build_umsb(>ac, src[0], ctx->ac.i32);
break;
case nir_op_ifind_msb:
-   src[0] = ac_to_integer(>ac, src[0]);
result = ac_build_imsb(>ac, src[0], ctx->ac.i32);
break;
case nir_op_uadd_carry:
-   src[0] = ac_to_integer(>ac, src[0]);
-   src[1] = ac_to_integer(>ac, src[1]);
result = emit_uint_carry(>ac, 
"llvm.uadd.with.overflow.i32", src[0], src[1]);
break;
case nir_op_usub_borrow:
-   src[0] = ac_to_integer(>ac, src[0]);
-   src[1] = ac_to_integer(>ac, src[1]);
result = emit_uint_carry(>ac, 
"llvm.usub.with.overflow.i32", src[0], src[1]);
break;
case nir_op_b2f16:
@@ -961,20 +950,15 @@ static void visit_alu(struct ac_nir_context *ctx, const 
nir_alu_instr *instr)
result = emit_b2i(>ac, src[0], 
instr->dest.dest.ssa.bit_size);
break;
case nir_op_i2b32:
-   src[0] = ac_to_integer(>ac, src[0]);
result = emit_i2b(>ac, src[0]);
break;
case nir_op_fquantize2f16:
result = emit_f2f16(>ac, src[0]);
break;
case nir_op_umul_high:
-   src[0] = ac_to_integer(>ac, src[0]);
-   src[1] = ac_to_integer(>ac, src[1]);
result = emit_umul_high(>ac, src[0], src[1]);
break;
case nir_op_imul_high:
-   src[0] = ac_to_integer(>ac, src[0]);
-   src[1] = ac_to_integer(>ac, src[1]);
result = emit_imul_high(>ac, src[0], src[1]);
break;
case nir_op_pack_half_2x16:


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] nir: initialise some variables in opt_if_loop_last_continue()

[Timothy Arceri]

Fixes a couple of Coverity warnings CID 1444626.

Fixes: e30804c6024f ("nir/radv: remove restrictions on 
opt_if_loop_last_continue()")
---
 src/compiler/nir/nir_opt_if.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
index 713bdf0c38a..d0aaf9f7133 100644
--- a/src/compiler/nir/nir_opt_if.c
+++ b/src/compiler/nir/nir_opt_if.c
@@ -839,8 +839,8 @@ static bool
 opt_if_loop_last_continue(nir_loop *loop, bool aggressive_last_continue)
 {
nir_if *nif;
-   bool then_ends_in_continue;
-   bool else_ends_in_continue;
+   bool then_ends_in_continue = false;
+   bool else_ends_in_continue = false;
 
/* Scan the control flow of the loop from the last to the first node
 * looking for an if-statement we can optimise.
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 6/6] glsl/linker: check for xfb_offset aliasing

[Timothy Arceri]

On 13/2/19 8:57 am, Andres Gomez wrote:

 From page 76 (page 80 of the PDF) of the GLSL 4.60 v.5 spec:

   " No aliasing in output buffers is allowed: It is a compile-time or
 link-time error to specify variables with overlapping transform
 feedback offsets."

Currently, this is expected to fail, but it succeeds:

   "

 ...

 layout (xfb_offset = 0) out vec2 a;
 layout (xfb_offset = 0) out vec4 b;

 ...

   "

v2: use a data structure to track the used components instead of a
 nested loop (Ilia).

Cc: Timothy Arceri 
Cc: Ilia Mirkin 
Signed-off-by: Andres Gomez 
---
  src/compiler/glsl/link_varyings.cpp | 89 ++---
  src/mesa/main/mtypes.h  |  3 +
  2 files changed, 70 insertions(+), 22 deletions(-)

diff --git a/src/compiler/glsl/link_varyings.cpp 
b/src/compiler/glsl/link_varyings.cpp
index 8c7d6c14c8f..95e9ae895d2 100644
--- a/src/compiler/glsl/link_varyings.cpp
+++ b/src/compiler/glsl/link_varyings.cpp
@@ -1173,6 +1173,73 @@ tfeedback_decl::store(struct gl_context *ctx, struct 
gl_shader_program *prog,
unsigned location = this->location;
unsigned location_frac = this->location_frac;
unsigned num_components = this->num_components();
+
+  /* From GL_EXT_transform_feedback:
+   *   A program will fail to link if:
+   *
+   * * the total number of components to capture is greater than the
+   *   constant MAX_TRANSFORM_FEEDBACK_INTERLEAVED_COMPONENTS_EXT and
+   *   the buffer mode is INTERLEAVED_ATTRIBS_EXT.
+   *
+   * From GL_ARB_enhanced_layouts:
+   *
+   *   "The resulting stride (implicit or explicit) must be less than or
+   *equal to the implementation-dependent constant
+   *gl_MaxTransformFeedbackInterleavedComponents."
+   */
+  if ((prog->TransformFeedback.BufferMode == GL_INTERLEAVED_ATTRIBS ||
+   has_xfb_qualifiers) &&
+  xfb_offset + num_components >
+  ctx->Const.MaxTransformFeedbackInterleavedComponents) {
+ linker_error(prog,
+  "The MAX_TRANSFORM_FEEDBACK_INTERLEAVED_COMPONENTS "
+  "limit has been exceeded.");
+ return false;
+  }
+
+  {


We don't need to do this in Mesa anymore. All compilers are now expected 
to support C99 i.e. variable declaration is no longer restricted to the 
start of a compound statement.



+ /* From the OpenGL 4.60.5 spec, section 4.4.2. Output Layout
+  * Qualifiers, Page 76, (Transform Feedback Layout Qualifiers):
+  *
+  * "No aliasing in output buffers is allowed: It is a compile-time or
+  *  link-time error to specify variables with overlapping transform
+  *  feedback offsets."
+  */
+ const unsigned max_components =
+ctx->Const.MaxTransformFeedbackInterleavedComponents;
+ const unsigned first_component = xfb_offset;
+ const unsigned last_component = xfb_offset + num_components - 1;
+ const unsigned start_word = BITSET_BITWORD(first_component);
+ const unsigned end_word = BITSET_BITWORD(last_component);
+ assert(last_component < max_components);
+
+ if (!info->Buffers[buffer].UsedComponents) {
+info->Buffers[buffer].UsedComponents =
+   rzalloc_array(info, BITSET_WORD, BITSET_WORDS(max_components));
+ }
+ BITSET_WORD *used = info->Buffers[buffer].UsedComponents;
+
+ for (unsigned word = start_word; word <= end_word; word++) {
+unsigned start_range = 0;
+unsigned end_range = BITSET_WORDBITS - 1;
+
+if (word == start_word)
+   start_range = first_component % BITSET_WORDBITS;
+
+if (word == end_word)
+   end_range = last_component % BITSET_WORDBITS;
+
+if (used[word] & BITSET_RANGE(start_range, end_range)) {
+   linker_error(prog,
+"variable '%s', xfb_offset (%d) "
+"is causing aliasing.",
+this->orig_name, xfb_offset * 4);
+   return false;
+}
+used[word] |= BITSET_RANGE(start_range, end_range);
+ }
+  }
+
while (num_components > 0) {
   unsigned output_size = MIN2(num_components, 4 - location_frac);
   assert((info->NumOutputs == 0 && max_outputs == 0) ||
@@ -1223,28 +1290,6 @@ tfeedback_decl::store(struct gl_context *ctx, struct 
gl_shader_program *prog,
info->Buffers[buffer].Stride = xfb_offset;
 }
  
-   /* From GL_EXT_transform_feedback:

-*   A program will fail to link if:
-*
-* * the total number of components to capture is greater than
-*   the constant MAX_TRANSFORM_FEEDBACK_INTERLEAVED_COMPONENT

Re: [Mesa-dev] [PATCH] glsl: allow the #extension directive within code blocks for the dri option

[Timothy Arceri]

Acked-by: Timothy Arceri 

On 10/4/19 9:39 am, Marek Olšák wrote:

From: Marek Olšák 

for Viewperf 13
---
  src/compiler/glsl/glsl_parser.yy | 9 +
  1 file changed, 9 insertions(+)

diff --git a/src/compiler/glsl/glsl_parser.yy b/src/compiler/glsl/glsl_parser.yy
index 8d6c47fb6a3..b91c24ebe97 100644
--- a/src/compiler/glsl/glsl_parser.yy
+++ b/src/compiler/glsl/glsl_parser.yy
@@ -2531,20 +2531,29 @@ statement_list:
 }
 | statement_list statement
 {
if ($2 == NULL) {
   _mesa_glsl_error(& @2, state, " statement");
   assert($2 != NULL);
}
$$ = $1;
$$->link.insert_before(& $2->link);
 }
+   | statement_list extension_statement
+   {
+  if (!state->allow_extension_directive_midshader) {
+ _mesa_glsl_error(& @1, state,
+  "#extension directive is not allowed "
+  "in the middle of a shader");
+ YYERROR;
+  }
+   }
 ;
  
  expression_statement:

 ';'
 {
void *ctx = state->linalloc;
$$ = new(ctx) ast_expression_statement(NULL);
$$->set_location(@1);
 }
 | expression ';'


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] glsl: fix shader_storage_blocks_write_access for SSBO block arrays

[Timothy Arceri]

Reviewed-by: Timothy Arceri 

On 9/4/19 7:21 am, Marek Olšák wrote:

From: Marek Olšák 

CTS: GL45-CTS.compute_shader.resources-max

Fixes: 4e1e8f684bf "glsl: remember which SSBOs are not read-only and pass it to 
gallium"
---
  src/compiler/glsl/link_uniforms.cpp | 8 ++--
  1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/compiler/glsl/link_uniforms.cpp 
b/src/compiler/glsl/link_uniforms.cpp
index ef124111991..bbd71593948 100644
--- a/src/compiler/glsl/link_uniforms.cpp
+++ b/src/compiler/glsl/link_uniforms.cpp
@@ -537,22 +537,26 @@ public:
  for (unsigned i = 0; i < num_blks; i++) {
 if (strcmp(var->get_interface_type()->name, blks[i].Name) == 
0) {
buffer_block_index = i;
break;
 }
  }
   }
   assert(buffer_block_index != -1);
  
   if (var->is_in_shader_storage_block() &&

- !var->data.memory_read_only)
-shader_storage_blocks_write_access |= 1 << buffer_block_index;
+ !var->data.memory_read_only) {
+shader_storage_blocks_write_access |=
+   u_bit_consecutive(buffer_block_index,
+ var->type->is_array() ?
+var->type->array_size() : 1);
+ }
  
   /* Uniform blocks that were specified with an instance name must be

* handled a little bit differently.  The name of the variable is the
* name used to reference the uniform block instead of being the name
* of a variable within the block.  Therefore, searching for the name
* within the block will fail.
*/
   if (var->is_interface_instance()) {
  ubo_byte_offset = 0;
  process(var->get_interface_type(),


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] nir/radv: remove restrictions on opt_if_loop_last_continue()

[Timothy Arceri]

When I implemented opt_if_loop_last_continue() I had restricted
this pass from moving other if-statements inside the branch opposite
the continue. At the time it was causing a bunch of spilling in
shader-db for i965.

However Samuel Pitoiset noticed that making this pass more aggressive
significantly improved the performance of Doom on RADV. Below are
the statistics he gathered.

28717 shaders in 14931 tests
Totals:
SGPRS: 1267317 -> 1267549 (0.02 %)
VGPRS: 896876 -> 895920 (-0.11 %)
Spilled SGPRs: 24701 -> 26367 (6.74 %)
Code Size: 48379452 -> 48507880 (0.27 %) bytes
Max Waves: 241159 -> 241190 (0.01 %)

Totals from affected shaders:
SGPRS: 23584 -> 23816 (0.98 %)
VGPRS: 25908 -> 24952 (-3.69 %)
Spilled SGPRs: 503 -> 2169 (331.21 %)
Code Size: 2471392 -> 2599820 (5.20 %) bytes
Max Waves: 586 -> 617 (5.29 %)

The codesize increases is related to Wolfenstein II it seems largely
due to an increase in phis rather than the existing jumps.

This gives +10% FPS with Doom on my Vega56.

Rhys Perry also benchmarked Doom on his VEGA64:

Before: 72.53 FPS
After:  80.77 FPS

v2: disable pass on non-AMD drivers

Reviewed-by: Ian Romanick  (v1)
---
 src/amd/vulkan/radv_shader.c |  2 +-
 src/compiler/nir/nir.h   |  2 +-
 src/compiler/nir/nir_opt_if.c| 87 
 src/freedreno/ir3/ir3_nir.c  |  2 +-
 src/gallium/auxiliary/nir/tgsi_to_nir.c  |  2 +-
 src/gallium/drivers/freedreno/a2xx/ir2_nir.c |  2 +-
 src/gallium/drivers/radeonsi/si_shader_nir.c |  2 +-
 src/intel/compiler/brw_nir.c |  2 +-
 src/mesa/state_tracker/st_glsl_to_nir.cpp|  2 +-
 9 files changed, 61 insertions(+), 42 deletions(-)

diff --git a/src/amd/vulkan/radv_shader.c b/src/amd/vulkan/radv_shader.c
index d3d073d1db8..7cde5e728e4 100644
--- a/src/amd/vulkan/radv_shader.c
+++ b/src/amd/vulkan/radv_shader.c
@@ -158,7 +158,7 @@ radv_optimize_nir(struct nir_shader *shader, bool 
optimize_conservatively,
NIR_PASS(progress, shader, nir_opt_remove_phis);
 NIR_PASS(progress, shader, nir_opt_dce);
 }
-NIR_PASS(progress, shader, nir_opt_if);
+NIR_PASS(progress, shader, nir_opt_if, true);
 NIR_PASS(progress, shader, nir_opt_dead_cf);
 NIR_PASS(progress, shader, nir_opt_cse);
 NIR_PASS(progress, shader, nir_opt_peephole_select, 8, true, 
true);
diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
index bc72d8f83f5..c1ecf5ad561 100644
--- a/src/compiler/nir/nir.h
+++ b/src/compiler/nir/nir.h
@@ -3434,7 +3434,7 @@ bool nir_opt_gcm(nir_shader *shader, bool value_number);
 
 bool nir_opt_idiv_const(nir_shader *shader, unsigned min_bit_size);
 
-bool nir_opt_if(nir_shader *shader);
+bool nir_opt_if(nir_shader *shader, bool aggressive_last_continue);
 
 bool nir_opt_intrinsics(nir_shader *shader);
 
diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
index 4d3183ed151..065fa83c51c 100644
--- a/src/compiler/nir/nir_opt_if.c
+++ b/src/compiler/nir/nir_opt_if.c
@@ -824,47 +824,60 @@ nir_block_ends_in_continue(nir_block *block)
  * The continue should then be removed by nir_opt_trivial_continues() and the
  * loop can potentially be unrolled.
  *
- * Note: do_work_2() is only ever blocks and nested loops. We could also nest
- * other if-statments in the branch which would allow further continues to
- * be removed. However in practice this can result in increased register
- * pressure.
+ * Note: Unless the function param aggressive_last_continue==true do_work_2()
+ * is only ever blocks and nested loops. We avoid nesting other if-statments
+ * in the branch as this can result in increased register pressure, and in
+ * the i965 driver it causes a large amount of spilling in shader-db.
+ * For RADV however nesting these if-statements allows further continues to be
+ * remove and provides a significant FPS boost in Doom, which is why we have
+ * opted for this special bool to enable for more aggresive optimisation.
+ * TODO: The GCM pass solves most of the spilling regressions in i965, if it
+ * is ever enabled we should consider removing the aggressive_last_continue
+ * param.
  */
 static bool
-opt_if_loop_last_continue(nir_loop *loop)
+opt_if_loop_last_continue(nir_loop *loop, bool aggressive_last_continue)
 {
-   /* Get the last if-stament in the loop */
+   nir_if *nif;
+   bool then_ends_in_continue;
+   bool else_ends_in_continue;
+
+   /* Scan the control flow of the loop from the last to the first node
+* looking for an if-statement we can optimise.
+*/
nir_block *last_block = nir_loop_last_block(loop);
nir_cf_node *if_node = nir_cf_node_prev(_block->cf_node);
while (if_node) {
-  if (if_node->type == nir_cf_node_if)
- break;
+  if (if_node->type == nir_cf_node_if) {
+ nif = nir_cf_node_as_if(if_node);
+ nir_block *then_block = 

Re: [Mesa-dev] [PATCH v2] nir: do not pack varying with different types

[Timothy Arceri]

Reviewed-by: Timothy Arceri 

On 5/4/19 8:26 pm, Samuel Pitoiset wrote:

The current algorithm only supports packing 32-bit types.
If a shader uses both 16-bit and 32-bit varyings, we shouldn't
compact them together.

v2 - fix regressions spotted by Intel CI

Cc: Timothy Arceri 
Signed-off-by: Samuel Pitoiset 
---
  src/compiler/nir/nir_linking_helpers.c | 13 +
  1 file changed, 13 insertions(+)

diff --git a/src/compiler/nir/nir_linking_helpers.c 
b/src/compiler/nir/nir_linking_helpers.c
index 146d4e4e591..7594728e25e 100644
--- a/src/compiler/nir/nir_linking_helpers.c
+++ b/src/compiler/nir/nir_linking_helpers.c
@@ -243,6 +243,7 @@ struct assigned_comps
 uint8_t comps;
 uint8_t interp_type;
 uint8_t interp_loc;
+   bool is_32bit;
  };
  
  /* Packing arrays and dual slot varyings is difficult so to avoid complex

@@ -308,6 +309,7 @@ get_unmoveable_components_masks(struct exec_list *var_list,
  comps[location + i].interp_type =
 get_interp_type(var, type, default_to_smooth_interp);
  comps[location + i].interp_loc = get_interp_loc(var);
+comps[location + i].is_32bit = glsl_type_is_32bit(type);
   }
}
 }
@@ -423,6 +425,7 @@ struct varying_component {
 nir_variable *var;
 uint8_t interp_type;
 uint8_t interp_loc;
+   bool is_32bit;
 bool is_patch;
 bool initialised;
  };
@@ -539,6 +542,7 @@ gather_varying_component_info(nir_shader *consumer,
  vc_info->interp_type =
 get_interp_type(in_var, type, default_to_smooth_interp);
  vc_info->interp_loc = get_interp_loc(in_var);
+vc_info->is_32bit = glsl_type_is_32bit(type);
  vc_info->is_patch = in_var->data.patch;
   }
}
@@ -572,6 +576,14 @@ assign_remap_locations(struct varying_loc (*remap)[4],
  continue;
   }
  
+ /* We can only pack varyings with matching types, and the current

+  * algorithm only supports packing 32-bit.
+  */
+ if (!assigned_comps[tmp_cursor].is_32bit) {
+tmp_comp = 0;
+continue;
+ }
+
   while (tmp_comp < 4 &&
  (assigned_comps[tmp_cursor].comps & (1 << tmp_comp))) {
  tmp_comp++;
@@ -589,6 +601,7 @@ assign_remap_locations(struct varying_loc (*remap)[4],
assigned_comps[tmp_cursor].comps |= (1 << tmp_comp);
assigned_comps[tmp_cursor].interp_type = info->interp_type;
assigned_comps[tmp_cursor].interp_loc = info->interp_loc;
+  assigned_comps[tmp_cursor].is_32bit = info->is_32bit;
  
/* Assign remap location */

remap[location][info->var->data.location_frac].component = tmp_comp++;


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] nir: do not pack varying with different types

[Timothy Arceri]

Seems ok for now.

Reviewed-by: Timothy Arceri 

On 3/4/19 10:37 pm, Samuel Pitoiset wrote:

The current algorithm only supports packing 32-bit types.
If a shader uses both 16-bit and 32-bit varyings, we shouldn't
compact them together.

Cc: Timothy Arceri 
Signed-off-by: Samuel Pitoiset 
---
  src/compiler/nir/nir_linking_helpers.c | 10 ++
  1 file changed, 10 insertions(+)

diff --git a/src/compiler/nir/nir_linking_helpers.c 
b/src/compiler/nir/nir_linking_helpers.c
index 146d4e4e591..482ac2881bc 100644
--- a/src/compiler/nir/nir_linking_helpers.c
+++ b/src/compiler/nir/nir_linking_helpers.c
@@ -243,6 +243,7 @@ struct assigned_comps
 uint8_t comps;
 uint8_t interp_type;
 uint8_t interp_loc;
+   uint8_t is_32bit;
  };
  
  /* Packing arrays and dual slot varyings is difficult so to avoid complex

@@ -308,6 +309,7 @@ get_unmoveable_components_masks(struct exec_list *var_list,
  comps[location + i].interp_type =
 get_interp_type(var, type, default_to_smooth_interp);
  comps[location + i].interp_loc = get_interp_loc(var);
+comps[location + i].is_32bit = 
glsl_type_is_32bit(glsl_without_array(type));
   }
}
 }
@@ -572,6 +574,14 @@ assign_remap_locations(struct varying_loc (*remap)[4],
  continue;
   }
  
+ /* We can only pack varyings with matching types, and the current

+  * algorithm only supports packing 32-bit.
+  */
+ if (!assigned_comps[tmp_cursor].is_32bit) {
+tmp_comp = 0;
+continue;
+ }
+
   while (tmp_comp < 4 &&
  (assigned_comps[tmp_cursor].comps & (1 << tmp_comp))) {
  tmp_comp++;


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v3] nir: propagate known constant values into the if-then branch

[Timothy Arceri]

Helps Max Waves / VGPR use in a bunch of Unigine Heaven
shaders.

shader-db results radeonsi (VEGA):
Totals from affected shaders:
SGPRS: 5505440 -> 5505872 (0.01 %)
VGPRS: 3077520 -> 3077296 (-0.01 %)
Spilled SGPRs: 39032 -> 39030 (-0.01 %)
Spilled VGPRs: 16326 -> 16326 (0.00 %)
Private memory VGPRs: 0 -> 0 (0.00 %)
Scratch size: 744 -> 744 (0.00 %) dwords per thread
Code Size: 123755028 -> 123753316 (-0.00 %) bytes
Compile Time: 2751028 -> 2560786 (-6.92 %) milliseconds
LDS: 1415 -> 1415 (0.00 %) blocks
Max Waves: 972192 -> 972240 (0.00 %)
Wait states: 0 -> 0 (0.00 %)

vkpipeline-db results RADV (VEGA):

Totals from affected shaders:
SGPRS: 160 -> 160 (0.00 %)
VGPRS: 88 -> 88 (0.00 %)
Spilled SGPRs: 0 -> 0 (0.00 %)
Spilled VGPRs: 0 -> 0 (0.00 %)
Private memory VGPRs: 0 -> 0 (0.00 %)
Scratch size: 0 -> 0 (0.00 %) dwords per thread
Code Size: 18268 -> 18152 (-0.63 %) bytes
LDS: 0 -> 0 (0.00 %) blocks
Max Waves: 26 -> 26 (0.00 %)
Wait states: 0 -> 0 (0.00 %)

v3: disable opt_for_known_values() for non scalar constants

Reviewed-by: Caio Marcelo de Oliveira Filho  (v2)
---
 src/compiler/nir/nir_opt_if.c | 61 +++
 1 file changed, 61 insertions(+)

diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
index 4d3183ed151..d8fa6b04b39 100644
--- a/src/compiler/nir/nir_opt_if.c
+++ b/src/compiler/nir/nir_opt_if.c
@@ -1326,6 +1326,66 @@ opt_if_merge(nir_if *nif)
return progress;
 }
 
+/* Perform optimisations based on the values we can derive from the evaluation
+ * of if-statement conditions.
+ */
+static bool
+opt_for_known_values(nir_builder *b, nir_if *nif)
+{
+   bool progress = false;
+
+   assert(nif->condition.is_ssa);
+   nir_ssa_def *if_cond = nif->condition.ssa;
+
+   if (if_cond->parent_instr->type != nir_instr_type_alu)
+  return false;
+
+   nir_alu_instr *alu = nir_instr_as_alu(if_cond->parent_instr);
+   switch (alu->op) {
+   case nir_op_feq:
+   case nir_op_ieq: {
+  nir_load_const_instr *load_const = NULL;
+  nir_ssa_def *unknown_val = NULL;
+
+  nir_ssa_def *src0 = alu->src[0].src.ssa;
+  nir_ssa_def *src1 = alu->src[1].src.ssa;
+  if (src0->parent_instr->type == nir_instr_type_load_const) {
+ load_const = nir_instr_as_load_const(src0->parent_instr);
+ unknown_val = src1;
+  } else if (src1->parent_instr->type == nir_instr_type_load_const) {
+ load_const = nir_instr_as_load_const(src1->parent_instr);
+ unknown_val = src0;
+  }
+
+  if (!load_const)
+return false;
+
+  /* TODO: remove this and support swizzles? */
+  if (unknown_val->num_components != 1 ||
+  load_const->def.num_components != 1)
+return false;
+
+  /* Replace unknown ssa uses with the known constant */
+  nir_foreach_use_safe(use_src, unknown_val) {
+ nir_cursor cursor = nir_before_src(use_src, false);
+ nir_block *use_block = nir_cursor_current_block(cursor);
+ if (nir_block_dominates(nir_if_first_then_block(nif), use_block)) {
+nir_instr_rewrite_src(use_src->parent_instr, use_src,
+  nir_src_for_ssa(_const->def));
+progress = true;
+ }
+  }
+
+  break;
+   }
+
+   default:
+  return false;
+   }
+
+   return progress;
+}
+
 static bool
 opt_if_cf_list(nir_builder *b, struct exec_list *cf_list)
 {
@@ -1380,6 +1440,7 @@ opt_if_safe_cf_list(nir_builder *b, struct exec_list 
*cf_list)
  progress |= opt_if_safe_cf_list(b, >then_list);
  progress |= opt_if_safe_cf_list(b, >else_list);
  progress |= opt_if_evaluate_condition_use(b, nif);
+ progress |= opt_for_known_values(b, nif);
  break;
   }
 
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] nir: disable opt_for_known_values() for non scalar constants

[Timothy Arceri]

We don't handle swizzles and non-scalar backends don't split these
constants so we need to skip these too.

Fixes: 4218b6422cf1 ("nir: propagate known constant values into the if-then 
branch")

https://bugs.freedesktop.org/show_bug.cgi?id=110311
---
 src/compiler/nir/nir_opt_if.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
index af63b90f249..d8fa6b04b39 100644
--- a/src/compiler/nir/nir_opt_if.c
+++ b/src/compiler/nir/nir_opt_if.c
@@ -1361,7 +1361,8 @@ opt_for_known_values(nir_builder *b, nir_if *nif)
 return false;
 
   /* TODO: remove this and support swizzles? */
-  if (unknown_val->num_components != 1)
+  if (unknown_val->num_components != 1 ||
+  load_const->def.num_components != 1)
 return false;
 
   /* Replace unknown ssa uses with the known constant */
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v2] nir: propagate known constant values into the if-then branch

[Timothy Arceri]

Helps Max Waves / VGPR use in a bunch of Unigine Heaven
shaders.

shader-db results radeonsi (VEGA):
Totals from affected shaders:
SGPRS: 5505440 -> 5505872 (0.01 %)
VGPRS: 3077520 -> 3077296 (-0.01 %)
Spilled SGPRs: 39032 -> 39030 (-0.01 %)
Spilled VGPRs: 16326 -> 16326 (0.00 %)
Private memory VGPRs: 0 -> 0 (0.00 %)
Scratch size: 744 -> 744 (0.00 %) dwords per thread
Code Size: 123755028 -> 123753316 (-0.00 %) bytes
Compile Time: 2751028 -> 2560786 (-6.92 %) milliseconds
LDS: 1415 -> 1415 (0.00 %) blocks
Max Waves: 972192 -> 972240 (0.00 %)
Wait states: 0 -> 0 (0.00 %)

vkpipeline-db results RADV (VEGA):

Totals from affected shaders:
SGPRS: 160 -> 160 (0.00 %)
VGPRS: 88 -> 88 (0.00 %)
Spilled SGPRs: 0 -> 0 (0.00 %)
Spilled VGPRs: 0 -> 0 (0.00 %)
Private memory VGPRs: 0 -> 0 (0.00 %)
Scratch size: 0 -> 0 (0.00 %) dwords per thread
Code Size: 18268 -> 18152 (-0.63 %) bytes
LDS: 0 -> 0 (0.00 %) blocks
Max Waves: 26 -> 26 (0.00 %)
Wait states: 0 -> 0 (0.00 %)

Reviewed-by: Caio Marcelo de Oliveira Filho 
---
 src/compiler/nir/nir_opt_if.c | 60 +++
 1 file changed, 60 insertions(+)

diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
index 4d3183ed151..af63b90f249 100644
--- a/src/compiler/nir/nir_opt_if.c
+++ b/src/compiler/nir/nir_opt_if.c
@@ -1326,6 +1326,65 @@ opt_if_merge(nir_if *nif)
return progress;
 }
 
+/* Perform optimisations based on the values we can derive from the evaluation
+ * of if-statement conditions.
+ */
+static bool
+opt_for_known_values(nir_builder *b, nir_if *nif)
+{
+   bool progress = false;
+
+   assert(nif->condition.is_ssa);
+   nir_ssa_def *if_cond = nif->condition.ssa;
+
+   if (if_cond->parent_instr->type != nir_instr_type_alu)
+  return false;
+
+   nir_alu_instr *alu = nir_instr_as_alu(if_cond->parent_instr);
+   switch (alu->op) {
+   case nir_op_feq:
+   case nir_op_ieq: {
+  nir_load_const_instr *load_const = NULL;
+  nir_ssa_def *unknown_val = NULL;
+
+  nir_ssa_def *src0 = alu->src[0].src.ssa;
+  nir_ssa_def *src1 = alu->src[1].src.ssa;
+  if (src0->parent_instr->type == nir_instr_type_load_const) {
+ load_const = nir_instr_as_load_const(src0->parent_instr);
+ unknown_val = src1;
+  } else if (src1->parent_instr->type == nir_instr_type_load_const) {
+ load_const = nir_instr_as_load_const(src1->parent_instr);
+ unknown_val = src0;
+  }
+
+  if (!load_const)
+return false;
+
+  /* TODO: remove this and support swizzles? */
+  if (unknown_val->num_components != 1)
+return false;
+
+  /* Replace unknown ssa uses with the known constant */
+  nir_foreach_use_safe(use_src, unknown_val) {
+ nir_cursor cursor = nir_before_src(use_src, false);
+ nir_block *use_block = nir_cursor_current_block(cursor);
+ if (nir_block_dominates(nir_if_first_then_block(nif), use_block)) {
+nir_instr_rewrite_src(use_src->parent_instr, use_src,
+  nir_src_for_ssa(_const->def));
+progress = true;
+ }
+  }
+
+  break;
+   }
+
+   default:
+  return false;
+   }
+
+   return progress;
+}
+
 static bool
 opt_if_cf_list(nir_builder *b, struct exec_list *cf_list)
 {
@@ -1380,6 +1439,7 @@ opt_if_safe_cf_list(nir_builder *b, struct exec_list 
*cf_list)
  progress |= opt_if_safe_cf_list(b, >then_list);
  progress |= opt_if_safe_cf_list(b, >else_list);
  progress |= opt_if_evaluate_condition_use(b, nif);
+ progress |= opt_for_known_values(b, nif);
  break;
   }
 
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] ac: fix return type for llvm.amdgcn.frexp.exp.i32.64

[Timothy Arceri]

Reviewed-by: Timothy Arceri 

On 29/3/19 6:39 pm, Samuel Pitoiset wrote:

This fixes the following piglit with RadeonSI
tests/spec/arb_gpu_shader_fp64/execution/built-in-functions/fs-frexp-dvec4.shader_test

Signed-off-by: Samuel Pitoiset 
---
  src/amd/common/ac_llvm_build.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/amd/common/ac_llvm_build.c b/src/amd/common/ac_llvm_build.c
index 5572b244720..eb9e4504248 100644
--- a/src/amd/common/ac_llvm_build.c
+++ b/src/amd/common/ac_llvm_build.c
@@ -3942,7 +3942,7 @@ ac_build_frexp_exp(struct ac_llvm_context *ctx, 
LLVMValueRef src0,
type = ctx->i32;
} else {
intr = "llvm.amdgcn.frexp.exp.i32.f64";
-   type = ctx->i64;
+   type = ctx->i32;
}
  
  	LLVMValueRef params[] = {



___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/4] ac: add ac_build_frex_exp() helper ans 16-bit/32-bit support

[Timothy Arceri]

This change broke radeonsi for:

tests/spec/arb_gpu_shader_fp64/execution/built-in-functions/fs-frexp-dvec4.shader_test

LLVM ERROR: Cannot select: intrinsic %llvm.amdgcn.frexp.exp

On 23/3/19 12:52 am, Samuel Pitoiset wrote:

Signed-off-by: Samuel Pitoiset 
---
  src/amd/common/ac_llvm_build.c  | 24 
  src/amd/common/ac_llvm_build.h  |  4 
  src/amd/common/ac_nir_to_llvm.c |  8 +---
  3 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/src/amd/common/ac_llvm_build.c b/src/amd/common/ac_llvm_build.c
index 4fd1d14b78f..5572b244720 100644
--- a/src/amd/common/ac_llvm_build.c
+++ b/src/amd/common/ac_llvm_build.c
@@ -3927,6 +3927,30 @@ ac_build_shuffle(struct ac_llvm_context *ctx, 
LLVMValueRef src, LLVMValueRef ind
  AC_FUNC_ATTR_CONVERGENT);
  }
  
+LLVMValueRef

+ac_build_frexp_exp(struct ac_llvm_context *ctx, LLVMValueRef src0,
+  unsigned bitsize)
+{
+   LLVMTypeRef type;
+   char *intr;
+
+   if (bitsize == 16) {
+   intr = "llvm.amdgcn.frexp.exp.i16.f16";
+   type = ctx->i16;
+   } else if (bitsize == 32) {
+   intr = "llvm.amdgcn.frexp.exp.i32.f32";
+   type = ctx->i32;
+   } else {
+   intr = "llvm.amdgcn.frexp.exp.i32.f64";
+   type = ctx->i64;
+   }
+
+   LLVMValueRef params[] = {
+   src0,
+   };
+   return ac_build_intrinsic(ctx, intr, type, params, 1,
+ AC_FUNC_ATTR_READNONE);
+}
  LLVMValueRef
  ac_build_frexp_mant(struct ac_llvm_context *ctx, LLVMValueRef src0,
unsigned bitsize)
diff --git a/src/amd/common/ac_llvm_build.h b/src/amd/common/ac_llvm_build.h
index db20b39d443..c3277fd2d13 100644
--- a/src/amd/common/ac_llvm_build.h
+++ b/src/amd/common/ac_llvm_build.h
@@ -677,6 +677,10 @@ ac_build_quad_swizzle(struct ac_llvm_context *ctx, 
LLVMValueRef src,
  LLVMValueRef
  ac_build_shuffle(struct ac_llvm_context *ctx, LLVMValueRef src, LLVMValueRef 
index);
  
+LLVMValueRef

+ac_build_frexp_exp(struct ac_llvm_context *ctx, LLVMValueRef src0,
+  unsigned bitsize);
+
  LLVMValueRef
  ac_build_frexp_mant(struct ac_llvm_context *ctx, LLVMValueRef src0,
unsigned bitsize);
diff --git a/src/amd/common/ac_nir_to_llvm.c b/src/amd/common/ac_nir_to_llvm.c
index 307a71c00ab..9331fd14b7d 100644
--- a/src/amd/common/ac_nir_to_llvm.c
+++ b/src/amd/common/ac_nir_to_llvm.c
@@ -803,9 +803,11 @@ static void visit_alu(struct ac_nir_context *ctx, const 
nir_alu_instr *instr)
break;
case nir_op_frexp_exp:
src[0] = ac_to_float(>ac, src[0]);
-   result = ac_build_intrinsic(>ac, 
"llvm.amdgcn.frexp.exp.i32.f64",
-   ctx->ac.i32, src, 1, 
AC_FUNC_ATTR_READNONE);
-
+   result = ac_build_frexp_exp(>ac, src[0],
+   ac_get_elem_bits(>ac, 
LLVMTypeOf(src[0])));
+   if (ac_get_elem_bits(>ac, LLVMTypeOf(src[0])) == 16)
+   result = LLVMBuildSExt(ctx->ac.builder, result,
+  ctx->ac.i32, "");
break;
case nir_op_frexp_sig:
src[0] = ac_to_float(>ac, src[0]);

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] nir: Lock around validation fail shader dumping

[Timothy Arceri]

Reviewed-by: Timothy Arceri 

On 28/3/19 3:22 am, Jason Ekstrand wrote:

---
  src/compiler/nir/nir_validate.c | 10 ++
  1 file changed, 10 insertions(+)

diff --git a/src/compiler/nir/nir_validate.c b/src/compiler/nir/nir_validate.c
index ef2e2b62783..f8327c0dd45 100644
--- a/src/compiler/nir/nir_validate.c
+++ b/src/compiler/nir/nir_validate.c
@@ -26,6 +26,7 @@
   */
  
  #include "nir.h"

+#include "c11/threads.h"
  #include 
  
  /*

@@ -1169,11 +1170,18 @@ destroy_validate_state(validate_state *state)
 _mesa_hash_table_destroy(state->errors, NULL);
  }
  
+mtx_t fail_dump_mutex = _MTX_INITIALIZER_NP;

+
  static void
  dump_errors(validate_state *state, const char *when)
  {
 struct hash_table *errors = state->errors;
  
+   /* Lock around dumping so that we get clean dumps in a multi-threaded

+* scenario
+*/
+   mtx_lock(_dump_mutex);
+
 if (when) {
fprintf(stderr, "NIR validation failed %s\n", when);
fprintf(stderr, "%d errors:\n", _mesa_hash_table_num_entries(errors));
@@ -1192,6 +1200,8 @@ dump_errors(validate_state *state, const char *when)
}
 }
  
+   mtx_unlock(_dump_mutex);

+
 abort();
  }
  


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] st/glsl_to_nir: fix incorrect arrary access

[Timothy Arceri]

On 26/3/19 3:43 am, Emil Velikov wrote:

Hi Timothy,

On Fri, 1 Mar 2019 at 10:36, Timothy Arceri  wrote:


This fixes a segfault when we try to access the array using a
-1 when the array wasn't allocated in the first place.

Before 7536af670b75 we would just access a pre-allocated array
that was also load/stored to/from the shader cache. But now the
cache will no longer allocate these arrays if they are empty.
The change resulted in tests such as the following segfaulting
when run with a warm shader cache.


I was going through some extra checks for 18.3 and noticed this commit.

Admittedly it does not fix 7536af670b75 although it does address a
preexisting issue exposed by it.
That said I suspect we'd want this for stable?


This fixes an issue that is not part of the default path of any drivers 
so I'm happy to skip back porting to stable.

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v4 1/6] glsl/linker: location aliasing requires types to have the same width

[Timothy Arceri]

On 21/3/19 2:07 am, Andres Gomez wrote:

On Wed, 2019-03-20 at 16:46 +0200, Andres Gomez wrote:

On Thu, 2019-03-21 at 00:20 +1100, Timothy Arceri wrote:

On 20/3/19 9:31 pm, Andres Gomez wrote:

On Wed, 2019-03-20 at 20:46 +1100, Timothy Arceri wrote:

On 2/2/19 5:05 am, Andres Gomez wrote:

From: Iago Toral Quiroga 

Regarding location aliasing requirements, the OpenGL spec says:

 "Further, when location aliasing, the aliases sharing the location
  must have the same underlying numerical type  (floating-point or
  integer)."

Khronos has further clarified that this also requires the underlying
types to have the same width, so we can't put a float and a double
in the same location slot for example. Future versions of the spec will
be corrected to make this clear.


The spec has always been very clear for example that it is ok to pack a
float with a dvec3:

   From section 4.4.1 (Input Layout Qualifiers) of the GLSL 4.6 spec:

"layout(location=8) in dvec3 h; // components 0,1,2 and 3 of location 8
   // and components 0 and 1 of location 9
layout(location=9, component=2) in float i; // okay, compts 2 and 3"


Mmmm ... I didn't notice that example before. I think it is just
incorrect or, rather, a typo. The inline comment says that the float
takes components 2 and 3. A float will count only for *1* component.
Hence, the intended type there is a double, in which case the aliasing
is allowed.

The spec is actually very clear just some paragraphs below:

  From Section 4.4.1 (Input Layout Qualifiers) of the GLSL 4.60 spec:

" Further, when location aliasing, the aliases sharing the location
  must have the same underlying numerical type and bit
  width (floating-point or integer, 32-bit versus 64-bit, etc.)
  and the same auxiliary storage and interpolation
  qualification. The one exception where component aliasing is
  permitted is for two input variables (not block members) to a
  vertex shader, which are allowed to have component aliasing. This
  vertex-variable component aliasing is intended only to support
  vertex shaders where each execution path accesses at most one
  input per each aliased component. Implementations are permitted,
  but not required, to generate link-time errors if they detect
  that every path through the vertex shader executable accesses
  multiple inputs aliased to any single component."


Yeah I noticed this when reviewing the piglit tests. It does seem we
need to fix this. However rather than a custom
get_numerical_sized_type() function we should be able to scrap it
completely and possibly future proof the code by using:

glsl_base_type_get_bit_size() and glsl_base_type_is_integer() instead
for the checks.

e.g.

info->is_integer =
glsl_base_type_is_integer(type->without_array()->base_type);
info->bit_size =
glsl_base_type_get_bit_size(type->without_array()->base_type);

And compare these instead.


I don't think this is a better option. Some of the reasons, as
commented in my other mail in this thread, are explained in the
previous (unfinished) review process for this patch.

Additionally, glsl_base_type_is_integer is only true for 32bit
integers. Also, only with these 2 checks, we would be failing for
images and samplers. Finally, we would end with quite a big comparison
that will have to be scattered in several points of the
check_location_aliasing function, making more difficult to understand
the logic behind it.


I disagree. IMO it would actually be much easier to follow and you can 
make the error message much more precise by including the bit-size and 
integer vs float difference.




Scratch the last paragraph. I committed the mistake of checking
"is_integer" instead of "glsl_base_type_is_integer"


This is all addressed at single point, the (already pre-existent)
get_numerical_sized_type function, which makes this easier to
understand and, IMHO, more future proof.


Even with the possibility of using those 2, I would rather keep the
get_numerical_sized_type to address the cases of "non-numerical"
variables beyond structs.


As per my reply to the other thread you shouldn't need to be testing for 
anything other than structs, everything else should already have been 
disallowed by compile-time errors. If I'm wrong can you please give me 
an example of what you are trying to fix.







Your going to need more information bug number etc to convince me this
change is correct.


I'll file a bug against the specs.

In the meanwhile, this is the expected behavior also in the CTS tests,
which is also confirmed with the nVIDIA blob.


This patch amends our implementation to account for this restriction.

In the process of doing this, I also noticed that we would attempt
to check aliasing requirements for record variables (including the test
for the numerical type) which is not allowed, inst

Re: [Mesa-dev] [PATCH v4 1/6] glsl/linker: location aliasing requires types to have the same width

[Timothy Arceri]

On 21/3/19 1:38 am, Andres Gomez wrote:

On Wed, 2019-03-20 at 20:46 +1100, Timothy Arceri wrote:

On 2/2/19 5:05 am, Andres Gomez wrote:

From: Iago Toral Quiroga 

Regarding location aliasing requirements, the OpenGL spec says:

"Further, when location aliasing, the aliases sharing the location
 must have the same underlying numerical type  (floating-point or
 integer)."

Khronos has further clarified that this also requires the underlying
types to have the same width, so we can't put a float and a double
in the same location slot for example. Future versions of the spec will
be corrected to make this clear.


The spec has always been very clear for example that it is ok to pack a
float with a dvec3:

  From section 4.4.1 (Input Layout Qualifiers) of the GLSL 4.6 spec:

"layout(location=8) in dvec3 h; // components 0,1,2 and 3 of location 8
  // and components 0 and 1 of location 9
   layout(location=9, component=2) in float i; // okay, compts 2 and 3"


Your going to need more information bug number etc to convince me this
change is correct.


This patch amends our implementation to account for this restriction.

In the process of doing this, I also noticed that we would attempt
to check aliasing requirements for record variables (including the test
for the numerical type) which is not allowed, instead, we should be
producing a linker error as soon as we see any attempt to do location
aliasing on non-numerical variables. For the particular case of structs,
we were producing a linker error in this case, but only because we
assumed that struct fields use all components in each location, so
any attempt to alias locations consumed by struct fields would produce
a link error due to component aliasing, which is not accurate of the
actual problem. This patch would make it produce an error for attempting
to alias a non-numerical variable instead, which is always accurate.


None of this should be needed at all. It is an error to use
location/component layouts on struct members.

"It is a compile-time error to use a location qualifier on a member of a
structure."

"It is a compile-time error to use *component* without also specifying
*location*"

So depending on the component aliasing check is sufficient. If you
really want to add a better error message then see my comments below.


I believe this is already answered in the previous (unfinished) review
process through which this patch went through. Specifically, the 2nd
review that gave place to the v3 patch.


No that doesn't address my comment. It shows why you did what you did, 
but what I'm saying is you don't need to check for this type of thing. 
It should all already be handled by compile-time error checking. You are 
making things more complicated than they need to be.


If I'm wrong please point me to a piglit test that can't be resolved by 
simply checking for a struct as I suggest below.



You can check it here:

https://lists.freedesktop.org/archives/mesa-dev/2017-November/175366.html
https://lists.freedesktop.org/archives/mesa-dev/2017-November/175573.html
https://lists.freedesktop.org/archives/mesa-dev/2017-November/175705.html



v2:
- Do not assert if we see invalid numerical types. These come
  straight from shader code, so we should produce linker errors if
  shaders attempt to do location aliasing on variables that are not
  numerical such as records.
- While we are at it, improve error reporting for the case of
  numerical type mismatch to include the shader stage.

v3:
- Allow location aliasing of images and samplers. If we get these
  it means bindless support is active and they should be handled
  as 64-bit integers (Ilia)
- Make sure we produce link errors for any non-numerical type
  for which we attempt location aliasing, not just structs.

v4:
- Rebased with minor fixes (Andres).
- Added fixing tag to the commit log (Andres).

Fixes: 13652e7516a ("glsl/linker: Fix type checks for location aliasing")
Cc: Ilia Mirkin 
Signed-off-by: Andres Gomez 
---
   src/compiler/glsl/link_varyings.cpp | 64 +
   1 file changed, 46 insertions(+), 18 deletions(-)

diff --git a/src/compiler/glsl/link_varyings.cpp 
b/src/compiler/glsl/link_varyings.cpp
index 3969c0120b3..3f41832ac93 100644
--- a/src/compiler/glsl/link_varyings.cpp
+++ b/src/compiler/glsl/link_varyings.cpp
@@ -424,15 +424,15 @@ compute_variable_location_slot(ir_variable *var, 
gl_shader_stage stage)
   
   struct explicit_location_info {

  ir_variable *var;
-   unsigned numerical_type;
+   int numerical_type;
  unsigned interpolation;
  bool centroid;
  bool sample;
  bool patch;
   };
   
-static inline unsigned

-get_numerical_type(const glsl_type *type)
+static inline int
+get_numerical_sized_type(const glsl_type *type)
   {
  /* From the OpenGL 4.6 spec, section 4.4.1 Input Layout

Re: [Mesa-dev] [PATCH v4 1/6] glsl/linker: location aliasing requires types to have the same width

[Timothy Arceri]

On 20/3/19 9:31 pm, Andres Gomez wrote:

On Wed, 2019-03-20 at 20:46 +1100, Timothy Arceri wrote:

On 2/2/19 5:05 am, Andres Gomez wrote:

From: Iago Toral Quiroga 

Regarding location aliasing requirements, the OpenGL spec says:

"Further, when location aliasing, the aliases sharing the location
 must have the same underlying numerical type  (floating-point or
 integer)."

Khronos has further clarified that this also requires the underlying
types to have the same width, so we can't put a float and a double
in the same location slot for example. Future versions of the spec will
be corrected to make this clear.


The spec has always been very clear for example that it is ok to pack a
float with a dvec3:

  From section 4.4.1 (Input Layout Qualifiers) of the GLSL 4.6 spec:

"layout(location=8) in dvec3 h; // components 0,1,2 and 3 of location 8
  // and components 0 and 1 of location 9
   layout(location=9, component=2) in float i; // okay, compts 2 and 3"


Mmmm ... I didn't notice that example before. I think it is just
incorrect or, rather, a typo. The inline comment says that the float
takes components 2 and 3. A float will count only for *1* component.
Hence, the intended type there is a double, in which case the aliasing
is allowed.

The spec is actually very clear just some paragraphs below:

 From Section 4.4.1 (Input Layout Qualifiers) of the GLSL 4.60 spec:

   " Further, when location aliasing, the aliases sharing the location
 must have the same underlying numerical type and bit
 width (floating-point or integer, 32-bit versus 64-bit, etc.)
 and the same auxiliary storage and interpolation
 qualification. The one exception where component aliasing is
 permitted is for two input variables (not block members) to a
 vertex shader, which are allowed to have component aliasing. This
 vertex-variable component aliasing is intended only to support
 vertex shaders where each execution path accesses at most one
 input per each aliased component. Implementations are permitted,
 but not required, to generate link-time errors if they detect
 that every path through the vertex shader executable accesses
 multiple inputs aliased to any single component."


Yeah I noticed this when reviewing the piglit tests. It does seem we 
need to fix this. However rather than a custom 
get_numerical_sized_type() function we should be able to scrap it 
completely and possibly future proof the code by using:


glsl_base_type_get_bit_size() and glsl_base_type_is_integer() instead 
for the checks.


e.g.

info->is_integer = 
glsl_base_type_is_integer(type->without_array()->base_type);
info->bit_size = 
glsl_base_type_get_bit_size(type->without_array()->base_type);


And compare these instead.




Your going to need more information bug number etc to convince me this
change is correct.


I'll file a bug against the specs.

In the meanwhile, this is the expected behavior also in the CTS tests,
which is also confirmed with the nVIDIA blob.




This patch amends our implementation to account for this restriction.

In the process of doing this, I also noticed that we would attempt
to check aliasing requirements for record variables (including the test
for the numerical type) which is not allowed, instead, we should be
producing a linker error as soon as we see any attempt to do location
aliasing on non-numerical variables. For the particular case of structs,
we were producing a linker error in this case, but only because we
assumed that struct fields use all components in each location, so
any attempt to alias locations consumed by struct fields would produce
a link error due to component aliasing, which is not accurate of the
actual problem. This patch would make it produce an error for attempting
to alias a non-numerical variable instead, which is always accurate.


None of this should be needed at all. It is an error to use
location/component layouts on struct members.

"It is a compile-time error to use a location qualifier on a member of a
structure."

"It is a compile-time error to use *component* without also specifying
*location*"

So depending on the component aliasing check is sufficient. If you
really want to add a better error message then see my comments below.



v2:
- Do not assert if we see invalid numerical types. These come
  straight from shader code, so we should produce linker errors if
  shaders attempt to do location aliasing on variables that are not
  numerical such as records.
- While we are at it, improve error reporting for the case of
  numerical type mismatch to include the shader stage.

v3:
- Allow location aliasing of images and samplers. If we get these
  it means bindless support is active and they should be handled
  as 64-bit integers (Ilia)
- Make sure we produce link errors for any non-numerical

Re: [Mesa-dev] [PATCH 1/1] nir: add a pass that removes continue blocks

[Timothy Arceri]

On 20/3/19 9:41 pm, Samuel Pitoiset wrote:

28717 shaders in 14931 tests
Totals:
SGPRS: 1267317 -> 1267549 (0.02 %)
VGPRS: 896876 -> 895920 (-0.11 %)
Spilled SGPRs: 24701 -> 26367 (6.74 %)
Code Size: 48379452 -> 48507880 (0.27 %) bytes
Max Waves: 241159 -> 241190 (0.01 %)

Totals from affected shaders:
SGPRS: 23584 -> 23816 (0.98 %)
VGPRS: 25908 -> 24952 (-3.69 %)
Spilled SGPRs: 503 -> 2169 (331.21 %)
Code Size: 2471392 -> 2599820 (5.20 %) bytes
Max Waves: 586 -> 617 (5.29 %)

The codesize increases is related to Wolfenstein II, maybe it has
too many continue blocks and we should add an arbitrary limit.

This gives +10% FPS with Doom on my Vega56.

Signed-off-by: Samuel Pitoiset 
---
  src/compiler/nir/nir_opt_if.c | 87 +++
  1 file changed, 87 insertions(+)

diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
index bc128f79f3c..47a8a65aad3 100644
--- a/src/compiler/nir/nir_opt_if.c
+++ b/src/compiler/nir/nir_opt_if.c
@@ -890,6 +890,92 @@ opt_if_loop_last_continue(nir_loop *loop)
 return true;
  }
  
+/**

+ * This optimization allows to remove continue blocks by moving the rest of the
+ * loop to the other side. This is only applied if continue blocks are in the
+ * top-level control flow of the loop. This turns:
+ *
+ *if (cond) {
+ *   ... code ...
+ *   continue
+ *} else {
+ *}
+ *... code ...
+ *
+ * into:
+ *
+ *if (cond) {
+ *   ... code ...
+ *   continue
+ *} else {
+ *   ... code ...
+ *}
+ */
+static bool
+opt_if_loop_remove_continue(nir_loop *loop)
+{
+   nir_block *header_block = nir_loop_first_block(loop);
+   nir_block *bottom_block = nir_loop_last_block(loop);
+   nir_block *prev_block =
+  nir_cf_node_as_block(nir_cf_node_prev(>cf_node));
+
+   /* It would be insane if this were not true */
+   assert(_mesa_set_search(header_block->predecessors, prev_block));
+
+   /* The loop must have, at least, two continue blocks (including the normal
+* continue at the end of loop).
+*/
+   if (header_block->predecessors->entries < 3)
+  return false;
+
+   /* Scan the control flow of the loop from the last to the first node, and
+* optimize when an if block contains a continue.
+*/
+   nir_cf_node *last_node = _block->cf_node;
+   nir_cf_node *first_node = _block->cf_node;
+
+   while (last_node != first_node) {
+  last_node = nir_cf_node_prev(last_node);
+  if (last_node->type != nir_cf_node_if)
+ continue;
+
+  nir_if *nif = nir_cf_node_as_if(last_node);
+  nir_block *then_block = nir_if_last_then_block(nif);
+
+  /* Nothing to do if the block isn't a continue block. */
+  nir_instr *instr = nir_block_last_instr(then_block);
+  if (!instr ||
+  instr->type != nir_instr_type_jump ||
+  nir_instr_as_jump(instr)->type != nir_jump_continue)
+ continue;


We also must check the other branch for a jump. Otherwise we risk changing:

if (cond) {
   ... code ...
   continue
} else {
   break;
}
... code ...

 into:

if (cond) {
   ... code ...
   continue
} else {
   break;
   ... code ...
}

It would be good if you could test out this branch [1] to see how it 
compares with this patch, it does the check and like the existing 
opt_if_loop_last_continue() also handles the alternate case of:


if (cond) {
   ... code ...
} else {
   ... code ...
   continue
}
... code ...


[1] https://gitlab.freedesktop.org/tarceri/mesa/commits/continue_opt


+
+  /* Get the block immediately following the if statement. */
+  nir_block *after_if_block =
+ nir_cf_node_as_block(nir_cf_node_next(>cf_node));
+
+  /* Nothing to do if the block following the if statement is empty. */
+  if (is_block_empty(after_if_block))
+ continue;
+
+  nir_cf_node *if_first_node = _if_block->cf_node;
+  nir_cf_node *if_last_node = _block->cf_node;
+  nir_block *else_block = nir_if_last_else_block(nif);
+
+  /* Move all nodes following the if statement to the last else block, in
+   * case there is complicated control flow.
+   */
+  nir_cf_list tmp;
+  nir_cf_extract(, nir_before_cf_node(if_first_node),
+   nir_after_cf_node(if_last_node));
+  nir_cf_reinsert(, nir_after_block(else_block));
+  nir_cf_delete();
+
+  return true;
+   }
+
+   return false;
+}
+
  /* Walk all the phis in the block immediately following the if statement and
   * swap the blocks.
   */
@@ -1355,6 +1441,7 @@ opt_if_cf_list(nir_builder *b, struct exec_list *cf_list)
   progress |= opt_simplify_bcsel_of_phi(b, loop);
   progress |= opt_peel_loop_initial_if(loop);
   progress |= opt_if_loop_last_continue(loop);
+ progress |= opt_if_loop_remove_continue(loop);
   break;
}
  


___
mesa-dev mailing list

Re: [Mesa-dev] [PATCH 1/1] nir: add a pass that removes continue blocks

[Timothy Arceri]

On 20/3/19 9:41 pm, Samuel Pitoiset wrote:

28717 shaders in 14931 tests
Totals:
SGPRS: 1267317 -> 1267549 (0.02 %)
VGPRS: 896876 -> 895920 (-0.11 %)
Spilled SGPRs: 24701 -> 26367 (6.74 %)
Code Size: 48379452 -> 48507880 (0.27 %) bytes
Max Waves: 241159 -> 241190 (0.01 %)

Totals from affected shaders:
SGPRS: 23584 -> 23816 (0.98 %)
VGPRS: 25908 -> 24952 (-3.69 %)
Spilled SGPRs: 503 -> 2169 (331.21 %)
Code Size: 2471392 -> 2599820 (5.20 %) bytes
Max Waves: 586 -> 617 (5.29 %)

The codesize increases is related to Wolfenstein II, maybe it has
too many continue blocks and we should add an arbitrary limit.

This gives +10% FPS with Doom on my Vega56.


Nice find. However as discussed on IRC this is just an unrestricted 
version of opt_if_loop_last_continue(), we should merge them but I would 
also like time to test this again with shader-db. The reason I 
restricted opt_if_loop_last_continue() was because it was causing extra 
register pressure in shaders. Its late so I wont be able to test until 
tomorrow.




Signed-off-by: Samuel Pitoiset 
---
  src/compiler/nir/nir_opt_if.c | 87 +++
  1 file changed, 87 insertions(+)

diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
index bc128f79f3c..47a8a65aad3 100644
--- a/src/compiler/nir/nir_opt_if.c
+++ b/src/compiler/nir/nir_opt_if.c
@@ -890,6 +890,92 @@ opt_if_loop_last_continue(nir_loop *loop)
 return true;
  }
  
+/**

+ * This optimization allows to remove continue blocks by moving the rest of the
+ * loop to the other side. This is only applied if continue blocks are in the
+ * top-level control flow of the loop. This turns:
+ *
+ *if (cond) {
+ *   ... code ...
+ *   continue
+ *} else {
+ *}
+ *... code ...
+ *
+ * into:
+ *
+ *if (cond) {
+ *   ... code ...
+ *   continue
+ *} else {
+ *   ... code ...
+ *}
+ */
+static bool
+opt_if_loop_remove_continue(nir_loop *loop)
+{
+   nir_block *header_block = nir_loop_first_block(loop);
+   nir_block *bottom_block = nir_loop_last_block(loop);
+   nir_block *prev_block =
+  nir_cf_node_as_block(nir_cf_node_prev(>cf_node));
+
+   /* It would be insane if this were not true */
+   assert(_mesa_set_search(header_block->predecessors, prev_block));
+
+   /* The loop must have, at least, two continue blocks (including the normal
+* continue at the end of loop).
+*/
+   if (header_block->predecessors->entries < 3)
+  return false;
+
+   /* Scan the control flow of the loop from the last to the first node, and
+* optimize when an if block contains a continue.
+*/
+   nir_cf_node *last_node = _block->cf_node;
+   nir_cf_node *first_node = _block->cf_node;
+
+   while (last_node != first_node) {
+  last_node = nir_cf_node_prev(last_node);
+  if (last_node->type != nir_cf_node_if)
+ continue;
+
+  nir_if *nif = nir_cf_node_as_if(last_node);
+  nir_block *then_block = nir_if_last_then_block(nif);
+
+  /* Nothing to do if the block isn't a continue block. */
+  nir_instr *instr = nir_block_last_instr(then_block);
+  if (!instr ||
+  instr->type != nir_instr_type_jump ||
+  nir_instr_as_jump(instr)->type != nir_jump_continue)
+ continue;
+
+  /* Get the block immediately following the if statement. */
+  nir_block *after_if_block =
+ nir_cf_node_as_block(nir_cf_node_next(>cf_node));
+
+  /* Nothing to do if the block following the if statement is empty. */
+  if (is_block_empty(after_if_block))
+ continue;
+
+  nir_cf_node *if_first_node = _if_block->cf_node;
+  nir_cf_node *if_last_node = _block->cf_node;
+  nir_block *else_block = nir_if_last_else_block(nif);
+
+  /* Move all nodes following the if statement to the last else block, in
+   * case there is complicated control flow.
+   */
+  nir_cf_list tmp;
+  nir_cf_extract(, nir_before_cf_node(if_first_node),
+   nir_after_cf_node(if_last_node));
+  nir_cf_reinsert(, nir_after_block(else_block));
+  nir_cf_delete();
+
+  return true;
+   }
+
+   return false;
+}
+
  /* Walk all the phis in the block immediately following the if statement and
   * swap the blocks.
   */
@@ -1355,6 +1441,7 @@ opt_if_cf_list(nir_builder *b, struct exec_list *cf_list)
   progress |= opt_simplify_bcsel_of_phi(b, loop);
   progress |= opt_peel_loop_initial_if(loop);
   progress |= opt_if_loop_last_continue(loop);
+ progress |= opt_if_loop_remove_continue(loop);
   break;
}
  


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v4 1/6] glsl/linker: location aliasing requires types to have the same width

[Timothy Arceri]

On 2/2/19 5:05 am, Andres Gomez wrote:

From: Iago Toral Quiroga 

Regarding location aliasing requirements, the OpenGL spec says:

   "Further, when location aliasing, the aliases sharing the location
must have the same underlying numerical type  (floating-point or
integer)."

Khronos has further clarified that this also requires the underlying
types to have the same width, so we can't put a float and a double
in the same location slot for example. Future versions of the spec will
be corrected to make this clear.


The spec has always been very clear for example that it is ok to pack a 
float with a dvec3:


From section 4.4.1 (Input Layout Qualifiers) of the GLSL 4.6 spec:

"layout(location=8) in dvec3 h; // components 0,1,2 and 3 of location 8
// and components 0 and 1 of location 9
 layout(location=9, component=2) in float i; // okay, compts 2 and 3"


Your going to need more information bug number etc to convince me this 
change is correct.




This patch amends our implementation to account for this restriction.

In the process of doing this, I also noticed that we would attempt
to check aliasing requirements for record variables (including the test
for the numerical type) which is not allowed, instead, we should be
producing a linker error as soon as we see any attempt to do location
aliasing on non-numerical variables. For the particular case of structs,
we were producing a linker error in this case, but only because we
assumed that struct fields use all components in each location, so
any attempt to alias locations consumed by struct fields would produce
a link error due to component aliasing, which is not accurate of the
actual problem. This patch would make it produce an error for attempting
to alias a non-numerical variable instead, which is always accurate.


None of this should be needed at all. It is an error to use 
location/component layouts on struct members.


"It is a compile-time error to use a location qualifier on a member of a 
structure."


"It is a compile-time error to use *component* without also specifying 
*location*"


So depending on the component aliasing check is sufficient. If you 
really want to add a better error message then see my comments below.





v2:
   - Do not assert if we see invalid numerical types. These come
 straight from shader code, so we should produce linker errors if
 shaders attempt to do location aliasing on variables that are not
 numerical such as records.
   - While we are at it, improve error reporting for the case of
 numerical type mismatch to include the shader stage.

v3:
   - Allow location aliasing of images and samplers. If we get these
 it means bindless support is active and they should be handled
 as 64-bit integers (Ilia)
   - Make sure we produce link errors for any non-numerical type
 for which we attempt location aliasing, not just structs.

v4:
   - Rebased with minor fixes (Andres).
   - Added fixing tag to the commit log (Andres).

Fixes: 13652e7516a ("glsl/linker: Fix type checks for location aliasing")
Cc: Ilia Mirkin 
Signed-off-by: Andres Gomez 
---
  src/compiler/glsl/link_varyings.cpp | 64 +
  1 file changed, 46 insertions(+), 18 deletions(-)

diff --git a/src/compiler/glsl/link_varyings.cpp 
b/src/compiler/glsl/link_varyings.cpp
index 3969c0120b3..3f41832ac93 100644
--- a/src/compiler/glsl/link_varyings.cpp
+++ b/src/compiler/glsl/link_varyings.cpp
@@ -424,15 +424,15 @@ compute_variable_location_slot(ir_variable *var, 
gl_shader_stage stage)
  
  struct explicit_location_info {

 ir_variable *var;
-   unsigned numerical_type;
+   int numerical_type;
 unsigned interpolation;
 bool centroid;
 bool sample;
 bool patch;
  };
  
-static inline unsigned

-get_numerical_type(const glsl_type *type)
+static inline int
+get_numerical_sized_type(const glsl_type *type)
  {
 /* From the OpenGL 4.6 spec, section 4.4.1 Input Layout Qualifiers, Page 
68,
  * (Location aliasing):
@@ -440,10 +440,25 @@ get_numerical_type(const glsl_type *type)
  *"Further, when location aliasing, the aliases sharing the location
  * must have the same underlying numerical type  (floating-point or
  * integer)
+*
+* Khronos has further clarified that this also requires the underlying
+* types to have the same width, so we can't put a float and a double
+* in the same location slot for example. Future versions of the spec will
+* be corrected to make this clear.
+*
+* Notice that we allow location aliasing for bindless image/samplers too
+* since these are defined as 64-bit integers.
  */
-   if (type->is_float() || type->is_double())
+   if (type->is_float())
return GLSL_TYPE_FLOAT;
-   return GLSL_TYPE_INT;
+   else if (type->is_integer())
+  return GLSL_TYPE_INT;
+   else if (type->is_double())
+  return GLSL_TYPE_DOUBLE;
+   else if (type->is_integer_64() 

Re: [Mesa-dev] [PATCH] nir: only override previous alu during loop analysis if supported

[Timothy Arceri]

Piglit test to trigger the crash:

https://patchwork.freedesktop.org/patch/292855/

On 19/3/19 12:09 pm, Timothy Arceri wrote:

Users of this function expect alu to be a supported comparision
if the induction variable is not NULL. Since we attempt to
override the return values if the first limit is not a const, we
must make sure we are dealing with a valid comparision before
overriding the alu instruction.

Fixes an unreachable in inverse_comparison() with the game
Assasins Creed Odyssey.

Fixes: 3235a942c16b ("nir: find induction/limit vars in iand instructions")
---
  src/compiler/nir/nir_loop_analyze.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/compiler/nir/nir_loop_analyze.c 
b/src/compiler/nir/nir_loop_analyze.c
index c8304611b28..cb71a55f2f1 100644
--- a/src/compiler/nir/nir_loop_analyze.c
+++ b/src/compiler/nir/nir_loop_analyze.c
@@ -847,9 +847,11 @@ try_find_trip_count_vars_in_iand(nir_alu_instr **alu,
 !is_var_constant(*limit)) {
src = iand->src[1].src.ssa;
if (src->parent_instr->type == nir_instr_type_alu) {
- *alu = nir_instr_as_alu(src->parent_instr);
- if (is_supported_terminator_condition(*alu))
+ nir_alu_instr *tmp_alu = nir_instr_as_alu(src->parent_instr);
+ if (is_supported_terminator_condition(tmp_alu)) {
+*alu = tmp_alu;
  *limit_rhs = get_induction_and_limit_vars(*alu, ind, limit, 
state);
+ }
}
 }
  }


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] nir: only override previous alu during loop analysis if supported

[Timothy Arceri]

Users of this function expect alu to be a supported comparision
if the induction variable is not NULL. Since we attempt to
override the return values if the first limit is not a const, we
must make sure we are dealing with a valid comparision before
overriding the alu instruction.

Fixes an unreachable in inverse_comparison() with the game
Assasins Creed Odyssey.

Fixes: 3235a942c16b ("nir: find induction/limit vars in iand instructions")
---
 src/compiler/nir/nir_loop_analyze.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/compiler/nir/nir_loop_analyze.c 
b/src/compiler/nir/nir_loop_analyze.c
index c8304611b28..cb71a55f2f1 100644
--- a/src/compiler/nir/nir_loop_analyze.c
+++ b/src/compiler/nir/nir_loop_analyze.c
@@ -847,9 +847,11 @@ try_find_trip_count_vars_in_iand(nir_alu_instr **alu,
!is_var_constant(*limit)) {
   src = iand->src[1].src.ssa;
   if (src->parent_instr->type == nir_instr_type_alu) {
- *alu = nir_instr_as_alu(src->parent_instr);
- if (is_supported_terminator_condition(*alu))
+ nir_alu_instr *tmp_alu = nir_instr_as_alu(src->parent_instr);
+ if (is_supported_terminator_condition(tmp_alu)) {
+*alu = tmp_alu;
 *limit_rhs = get_induction_and_limit_vars(*alu, ind, limit, state);
+ }
   }
}
 }
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] ac/nir_to_llvm: make sure to we are comparing ints in a couple of places

[Timothy Arceri]

Fixes llvm validation error in a Deus Ex shader on radeonsi.

Both operands to ICmp instruction are not of the same type!
---
 src/amd/common/ac_nir_to_llvm.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/amd/common/ac_nir_to_llvm.c b/src/amd/common/ac_nir_to_llvm.c
index 0ca3f83a248..1dfb865a41b 100644
--- a/src/amd/common/ac_nir_to_llvm.c
+++ b/src/amd/common/ac_nir_to_llvm.c
@@ -195,7 +195,10 @@ static LLVMValueRef emit_int_cmp(struct ac_llvm_context 
*ctx,
  LLVMIntPredicate pred, LLVMValueRef src0,
  LLVMValueRef src1)
 {
-   LLVMValueRef result = LLVMBuildICmp(ctx->builder, pred, src0, src1, "");
+   LLVMValueRef result;
+   src0 = ac_to_integer(ctx, src0);
+   src1 = ac_to_integer(ctx, src1);
+   result = LLVMBuildICmp(ctx->builder, pred, src0, src1, "");
return LLVMBuildSelect(ctx->builder, result,
   LLVMConstInt(ctx->i32, 0x, false),
   ctx->i32_0, "");
@@ -281,6 +284,8 @@ static LLVMValueRef emit_minmax_int(struct ac_llvm_context 
*ctx,
LLVMIntPredicate pred,
LLVMValueRef src0, LLVMValueRef src1)
 {
+   src0 = ac_to_integer(ctx, src0);
+   src1 = ac_to_integer(ctx, src1);
return LLVMBuildSelect(ctx->builder,
   LLVMBuildICmp(ctx->builder, pred, src0, src1, 
""),
   src0,
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] ac/nir_to_llvm: add assert to emit_bcsel()

[Timothy Arceri]

nir to llvm assumes we have already split vectors to scalars via
nir_lower_alu_to_scalar().
---
 src/amd/common/ac_nir_to_llvm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/amd/common/ac_nir_to_llvm.c b/src/amd/common/ac_nir_to_llvm.c
index 18297ed99b1..0ca3f83a248 100644
--- a/src/amd/common/ac_nir_to_llvm.c
+++ b/src/amd/common/ac_nir_to_llvm.c
@@ -268,6 +268,8 @@ static LLVMValueRef emit_intrin_3f_param(struct 
ac_llvm_context *ctx,
 static LLVMValueRef emit_bcsel(struct ac_llvm_context *ctx,
   LLVMValueRef src0, LLVMValueRef src1, 
LLVMValueRef src2)
 {
+   assert(LLVMGetTypeKind(LLVMTypeOf(src0)) != LLVMVectorTypeKind);
+
LLVMValueRef v = LLVMBuildICmp(ctx->builder, LLVMIntNE, src0,
   ctx->i32_0, "");
return LLVMBuildSelect(ctx->builder, v,
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radv: remove sisched hack for talos

[Timothy Arceri]

On 16/3/19 6:53 am, Dieter Nützel wrote:

Am 15.03.2019 15:20, schrieb Samuel Pitoiset:

Results of my benchmarks are:

3 runs at 1080p:

GFX8: -1%

GFX9: -1.12%

3 runs at 4k:

GFX8: -2%

GFX9: -1.85%

I'm actually not sure if we want to remove it...


Yes, my hint is we should wait until Marek is back from vacation.
Running all the time with AMD_DEBUB (R600_DEBUG)=nir,sisched 'cause it's
worth it...


Can you point to specific games that are helped?



Dieter


On 3/15/19 11:25 AM, Timothy Arceri wrote:

This was added in 8a7d4092d260 but no longer seems to have any
impact on performance.
---
  src/amd/vulkan/radv_device.c | 10 +-
  1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/src/amd/vulkan/radv_device.c b/src/amd/vulkan/radv_device.c
index 9570c15af02..56421dbc74b 100644
--- a/src/amd/vulkan/radv_device.c
+++ b/src/amd/vulkan/radv_device.c
@@ -499,15 +499,7 @@ radv_handle_per_app_options(struct radv_instance 
*instance,

  if (!name)
  return;
  -    if (!strcmp(name, "Talos - Linux - 32bit") ||
-    !strcmp(name, "Talos - Linux - 64bit")) {
-    if (!(instance->debug_flags & RADV_DEBUG_NO_SISCHED)) {
-    /* Force enable LLVM sisched for Talos because it looks
- * safe and it gives few more FPS.
- */
-    instance->perftest_flags |= RADV_PERFTEST_SISCHED;
-    }
-    } else if (!strcmp(name, "DOOM_VFR")) {
+    if (!strcmp(name, "DOOM_VFR")) {
  /* Work around a Doom VFR game bug */
  instance->debug_flags |= RADV_DEBUG_NO_DYNAMIC_BOUNDS;
  }

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev