The intention is that things like
int;
will generate a warning. However, we were also accidentally emitting
the same warning for things like
struct Foo { int x; };
Signed-off-by: Ian Romanick <ian.d.romanick@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=68838
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
Cc: Aras Pranckevicius <aras@unity3d.com>
Cc: "9.2 10.0" <mesa-stable@lists.freedesktop.org>
Previously, we stored an array of up to 16 additional shaders to link,
as well as a count of how many each shader actually needed.
Since the built-in functions rewrite, all the built-ins are stored in a
single shader. So all we need is a boolean indicating whether a shader
needs to link against built-ins or not.
During linking, we can avoid creating the temporary array if none of the
shaders being linked need built-ins. Otherwise, it's simply a copy of
the array that has one additional element. This is much simpler.
This patch saves approximately 128 bytes of memory per gl_shader object.
Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
Since the built-in functions rewrite, num_builtins_to_link is always either
0 or 1, so we don't need tho crazy loop starting at -1 with a special
case.
All we need to do is print the prototypes from the current shader, and
the single built-in function shader (if present).
Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
Previously, when we hit a "no matching function" error, it looked like:
0:0(0): error: no matching function for call to `cos()'
0:0(0): error: candidates are: float cos(float)
0:0(0): error: vec2 cos(vec2)
0:0(0): error: vec3 cos(vec3)
0:0(0): error: vec4 cos(vec4)
Now it looks like:
0:0(0): error: no matching function for call to `cos()'; candidates are:
0:0(0): error: float cos(float)
0:0(0): error: vec2 cos(vec2)
0:0(0): error: vec3 cos(vec3)
0:0(0): error: vec4 cos(vec4)
This is not really any worse and removes the need for the prefix variable.
It will also help with the next commit's refactoring.
Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
Reviewed-by: Chris Forbes <chrisf@ijw.co.nz>
Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
There's no need to loop through the "parameters" list and remove every
element; move_nodes_to(¶meters) already throws away all elements of
the destination list.
Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
The compiler back-ends (i965's fs_visitor and brw_visitor,
ir_to_mesa_visitor, and glsl_to_tgsi_visitor) have been assuming this
for some time. Thanks to the preceding patch, the compiler front-end
no longer breaks this assumption.
This patch adds code to validate the assumption so that if we have
future bugs, we'll be able to catch them earlier.
Reviewed-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
The compiler back-ends (i965's fs_visitor and brw_visitor,
ir_to_mesa_visitor, and glsl_to_tgsi_visitor) assume that when
ir_loop::counter is non-null, it points to a fresh ir_variable that
should be used as the loop counter (as opposed to an ir_variable that
exists elsewhere in the instruction stream).
However, previous to this patch:
(1) loop_control_visitor did not create a new variable for
ir_loop::counter; instead it re-used the existing ir_variable.
This caused the loop counter to be double-incremented (once
explicitly by the body of the loop, and once implicitly by
ir_loop::increment).
(2) ir_clone did not clone ir_loop::counter properly, resulting in the
cloned ir_loop pointing to the source ir_loop's counter.
(3) ir_hierarchical_visitor did not visit ir_loop::counter, resulting
in the ir_variable being missed by reparenting.
Additionally, most optimization passes (e.g. loop unrolling) assume
that the variable mentioned by ir_loop::counter is not accessed in the
body of the loop (an assumption which (1) violates).
The combination of these factors caused a perfect storm in which the
code worked properly nearly all of the time: for loops that got
unrolled, (1) would introduce a double-increment, but loop unrolling
would fail to notice it (since it assumes that ir_loop::counter is not
accessed in the body of the loop), so it would unroll the loop the
correct number of times. For loops that didn't get unrolled, (1)
would introduce a double-increment, but then later when the IR was
cloned for linking, (2) would prevent the loop counter from being
cloned properly, so it would look to further analysis stages like an
independent variable (and hence the double-increment would stop
occurring). At the end of linking, (3) would prevent the loop counter
from being reparented, so it would still belong to the shader object
rather than the linked program object. Provided that the client
program didn't delete the shader object, the memory would never get
reclaimed, and so the shader would function properly.
However, for loops that didn't get unrolled, if the client program did
delete the shader object, and the memory belonging to the loop counter
got re-used, this could cause a use-after-free bug, leading to a
crash.
This patch fixes loop_control_visitor, ir_clone, and
ir_hierarchical_visitor to treat ir_loop::counter the same way the
back-ends treat it: as a freshly allocated ir_variable that needs to
be visited and cloned independently of other ir_variables.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=72026
Reviewed-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
If an ir_loop has a non-null "counter" field, the variable referred to
by this field is implicitly read and written by the loop. We need to
account for this in ir_variable_refcount, otherwise there is a danger
we will try to dead-code-eliminate the loop counter variable.
Note: at the moment the dead code elimination bug doesn't occur due to
a bug in ir_hierarchical_visitor: it doesn't visit the "counter"
field, so dead code elimination doesn't treat it as a candidate for
elimination. But the patch to follow will fix that bug, so we need to
fix ir_variable_refcount first in order to avoid breaking dead code
elimination.
Reviewed-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
This patch fixes this build error with Oracle Solaris Studio.
libtool: link: /opt/solarisstudio12.3/bin/cc -g -o glcpp/glcpp glcpp.o prog_hash_table.o ./.libs/libglcpp.a
Undefined first referenced
symbol in file
sqrt prog_hash_table.o
Signed-off-by: Vinson Lee <vlee@freedesktop.org>
Reviewed-by: Brian Paul <brianp@vmware.com>
Reviewed-by: Matt Turner <mattst88@gmail.com>
If reparent_ir() is called on invalid IR, then there's a danger that
it will fail to reparent all of the necessary nodes. For example, if
the IR contains an ir_dereference_variable which refers to an
ir_variable that's not in the tree, that ir_variable won't get
reparented, resulting in subtle use-after-free bugs once the
non-reparented nodes are freed. (This is exactly what happened in the
bug fixed by the previous commit).
This patch makes this kind of bug far easier to track down, by
transforming it from a use-after-free bug into an explicit IR
validation error.
Reviewed-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
In commit 065da16 (glsl: Convert lower_clip_distance_visitor to be an
ir_rvalue_visitor), we failed to notice that since
lower_clip_distance_visitor overrides visit_leave(ir_assignment *),
ir_rvalue_visitor::visit_leave(ir_assignment *) wasn't getting called.
As a result, clip distance dereferences appearing directly on the
right hand side of an assignment (not in a subexpression) weren't
getting properly lowered. This caused an ir_dereference_variable node
to be left in the IR that referred to the old gl_ClipDistance
variable. However, since the lowering pass replaces gl_ClipDistance
with gl_ClipDistanceMESA, this turned into a dangling pointer when the
IR got reparented.
Prior to the introduction of geometry shaders, this bug was unlikely
to arise, because (a) reading from gl_ClipDistance[i] in the fragment
shader was rare, and (b) when it happened, it was likely that it would
either appear in a subexpression, or be hoisted into a subexpression
by tree grafting.
However, in a geometry shader, we're likely to see a statement like
this, which would trigger the bug:
gl_ClipDistance[i] = gl_in[j].gl_ClipDistance[i];
This patch causes
lower_clip_distance_visitor::visit_leave(ir_assignment *) to call the
base class visitor, so that the right hand side of the assignment is
properly lowered.
Fixes piglit test:
- spec/glsl-1.50/execution/geometry/clip-distance-itemized-copy
Cc: Ian Romanick <idr@freedesktop.org>
Cc: "9.2" <mesa-stable@lists.freedesktop.org>
Cc: "10.0" <mesa-stable@lists.freedesktop.org>
Reviewed-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
V2: Return after error to avoid cascading error messages and
removed redundant "to" from error message
Signed-off-by: Timothy Arceri <t_arceri@yahoo.com.au>
Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
The canary is basically just to give a better debugging message when you
ralloc_free() something that wasn't rallocated. Reduces maximum memory
usage of apitrace replay of the dota2 demo by 60MB on my 64-bit system (so
half that on a real 32-bit dota2 environment).
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
Previously, we checked for interstage uniform interface block link
errors in validate_interstage_interface_blocks(), which is only called
on pairs of adjacent shader stages. Therefore, we failed to detect
uniform interface block mismatches between non-adjacent shader stages.
Before the introduction of geometry shaders, this wasn't a problem,
because the only supported shader stages were vertex and fragment
shaders, therefore they were always adjacent. However, now that we
allow a program to contain vertex, geometry, and fragment shaders,
that is no longer the case.
Fixes piglit test "skip-stage-uniform-block-array-size-mismatch".
Cc: "10.0" <mesa-stable@lists.freedesktop.org>
v2: Rename validate_interstage_interface_blocks() to
validate_interstage_inout_blocks() to reflect the fact that it no
longer validates uniform blocks.
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
v3: Make validate_interstage_inout_blocks() skip uniform blocks.
Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
Previously, when attempting to link a vertex shader and a geometry
shader that use different GLSL versions, we would sometimes generate a
link error due to the implicit declaration of gl_PerVertex being
different between the two GLSL versions.
This patch fixes that problem by only requiring interface block
definitions to match when they are explicitly declared.
Fixes piglit test "shaders/version-mixing vs-gs".
Cc: "10.0" <mesa-stable@lists.freedesktop.org>
v2: In the interface_block_definition constructor, move the assignment
to explicitly_declared after the existing if block.
Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
From section 7.1 (Built-In Language Variables) of the GLSL 4.10
spec:
Also, if a built-in interface block is redeclared, no member of
the built-in declaration can be redeclared outside the block
redeclaration.
We have been regarding this text as a clarification to the behaviour
established for gl_PerVertex by GLSL 1.50, so we apply it regardless
of GLSL version.
This patch enforces the rule by adding an enum to ir_variable to track
how the variable was declared: implicitly, normally, or in an
interface block.
Fixes piglit tests:
- gs-redeclares-pervertex-out-after-global-redeclaration.geom
- vs-redeclares-pervertex-out-after-global-redeclaration.vert
- gs-redeclares-pervertex-out-after-other-global-redeclaration.geom
- vs-redeclares-pervertex-out-after-other-global-redeclaration.vert
- gs-redeclares-pervertex-out-before-global-redeclaration
- vs-redeclares-pervertex-out-before-global-redeclaration
Cc: "10.0" <mesa-stable@lists.freedesktop.org>
v2: Don't set "how_declared" redundantly in builtin_variables.cpp.
Properly clone "how_declared".
Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
Fixes 'make check' on distros where bash is not at /bin/bash.
Signed-off-by: Vinson Lee <vlee@freedesktop.org>
Reviewed-by: Brian Paul <brianp@vmware.com>
Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
Tested-by: Ian Romanick <ian.d.romanick@intel.com>
The comment was stale, because the lowering in question wasn't happening
in lower_instructions.cpp. Presumably if the lowering ever moves there,
we can plumb the lowering mask through to opt_algebraic.
total instructions in shared programs: 1618696 -> 1616810 (-0.12%)
instructions in affected programs: 243018 -> 241132 (-0.78%)
GAINED: 0
LOST: 0
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
Previously, when doing intrastage and interstage interface block
linking, we only checked the interface type; this prevented us from
catching some link errors.
We now check the following additional constraints:
- For intrastage linking, the presence/absence of interface names must
match.
- For shader ins/outs, the interface names themselves must match when
doing intrastage linking (note: it's not clear from the spec whether
this is necessary, but Mesa's implementation currently relies on
it).
- Array vs. nonarray must be consistent, taking into account the
special rules for vertex-geometry linkage.
- Array sizes must be consistent (exception: during intrastage
linking, an unsized array matches a sized array).
Note: validate_interstage_interface_blocks currently handles both
uniforms and in/out variables. As a result, if all three shader types
are present (VS, GS, and FS), and a uniform interface block is
mentioned in the VS and FS but not the GS, it won't be validated. I
plan to address this in later patches.
Fixes the following piglit tests in spec/glsl-1.50/linker:
- interface-blocks-vs-fs-array-size-mismatch
- interface-vs-array-to-fs-unnamed
- interface-vs-unnamed-to-fs-array
- intrastage-interface-unnamed-array
v2: Simplify logic in intrastage_match() for handling array sizes.
Make extra_array_level const. Use an unnamed temporary
interface_block_definition in validate_interstage_interface_blocks()'s
first call to definitions->store().
Cc: "10.0" <mesa-stable@lists.freedesktop.org>
Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
v2: Add comments on the purpose of the auxiliary data structures.
Check for atomic counter overlaps. Use the contains_atomic()
convenience method. Add static assert with the number of expected
shader stages.
v3: Don't resize atomic arrays.
v4: Add comment on the reason why we don't resize atomic counter
arrays. Use 'strcmp(...) == 0' instead of '!strcmp(...)'.
v5 (idr): Don't use STL in the linker.
Signed-off-by: Francisco Jerez <currojerez@riseup.net>
Signed-off-by: Ian Romanick <ian.d.romanick@intel.com>
Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
v2: Mark atomic counters as read-only variables. Move offset overlap
code to the linker. Use the contains_atomic() convenience method.
v3: Use pointer to integer instead of non-const reference. Add
comment so we remember to add a spec quotation from the next GLSL
release once the issue of atomic counter aggregation within
structures is clarified.
v4 (idr): Don't use std::map because it's overkill. Add an assertion
that ctx->Const.MaxAtomicBufferBindings <= MAX_COMBINED_ATOMIC_BUFFERS.
Signed-off-by: Francisco Jerez <currojerez@riseup.net>
Signed-off-by: Ian Romanick <ian.d.romanick@intel.com>
Reviewed-by: Paul Berry <stereotype441@gmail.com>
Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
Previously, we only exposed them in desktop GL or with:
#extension GL_OES_standard_derivatives : enable
GLSL ES 3.00 includes these without an extension, so we need to expose
them by default.
Note that the above #extension line results in an error or desktop GL,
so we don't need to worry about this.
Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
Reviewed-by: Matt Turner <mattst88@gmail.com>
Previously, when packing geometry shader input varyings like this:
in float foo[3];
in float bar[3];
lower_packed_varyings would declare a packed varying like this:
(declare (shader_in flat) (array ivec4 3) packed:foo[0],bar[0])
That's confusing, since the packed varying acutally stores all three
values of foo and all three values of bar.
This patch causes it to generate the more sensible declaration:
(declare (shader_in flat) (array ivec4 3) packed:foo,bar)
Note that there should be no functional change for users of geometry
shaders, since the packed name is only used for generating debug
output. But this should reduce confusion when using INTEL_DEBUG=gs.
Reviewed-by: Eric Anholt <eric@anholt.net>
New builtins added by GL_ARB_sample_shading:
in vec2 gl_SamplePosition
in int gl_SampleID
in int gl_NumSamples
out int gl_SampleMask[]
V2: - Use SWIZZLE_XXXX for STATE_NUM_SAMPLES.
- Use "result.samplemask" in arb_output_attrib_string.
- Add comment to explain the size of gl_SampleMask[] array.
- Make gl_SampleID and gl_SamplePosition system values.
Signed-off-by: Anuj Phogat <anuj.phogat@gmail.com>
Reviewed-by: Paul Berry <stereotype441@gmail.com>
Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
This patch implements the common support code required for the
GL_ARB_sample_shading extension.
V2: Move GL_ARB_sample_shading to ARB extension list.
Signed-off-by: Anuj Phogat <anuj.phogat@gmail.com>
Reviewed-by: Ian Romanick <idr@freedesktop.org>
Reviewed-by: Ken Graunke <kenneth@whitecape.org>
This only operates on constant/uniform values for now, because otherwise I'd
have to deal with killing my available CSE entries when assignments happen,
and getting even this working in the tree ir was painful enough.
As is, it has the following effect in shader-db:
total instructions in shared programs: 1524077 -> 1521964 (-0.14%)
instructions in affected programs: 50629 -> 48516 (-4.17%)
GAINED: 0
LOST: 0
And, for tropics, that accounts for most of the effect, the FPS
improvement is 11.67% +/- 0.72% (n=3).
v2: Use read_only field of the variable, manually check the lod_info union
members, use get_num_operands(), rename cse_operands_visitor to
is_cse_candidate_visitor, move all is-a-candidate logic to that
function, and call it before checking for CSE on a given rvalue, more
comments, use private keyword.
Reviewed-by: Paul Berry <stereotype441@gmail.com>
This will simplify the addition of layout(location) qualifiers for
separate shader objects. This was validated with new piglit tests
arb_explicit_attrib_location/1.30/compiler/not-enabled-01.vert and
arb_explicit_attrib_location/1.30/compiler/not-enabled-02.vert.
v2: Refactor error checking to check_explicit_attrib_location_allowed
and eliminate the gotos. Suggested by Paul.
Signed-off-by: Ian Romanick <ian.d.romanick@intel.com>
Reviewed-by: Paul Berry <stereotype441@gmail.com>
Use mode_string to get the name of the variable mode. Slightly change
the control flow. Both of these changes make it easier to support
separate shader object location layouts.
The format of the message changed because mode_string can return a
string like "shader output". This would result in an awkward message
like "vertex shader shader output..."
Signed-off-by: Ian Romanick <ian.d.romanick@intel.com>
Reviewed-by: Paul Berry <stereotype441@gmail.com>
I made this a function (instead of a method of ir_variable) because it
made the change set smaller, and I expect that there will be an overload
that takes an ir_var_mode enum. Having both functions used the same way
seemed better.
v2: Add missing case for ir_var_system_value.
v3: Change the ir_var_mode_count case to just break. Move the assertion
and the return outside the switch-statment. In the unlikely event that
var->mode is an invalid value other than ir_var_mode_count, the
assertion will still fire, and in release builds we won't wind up
returning a garbage pointer. Suggested by Paul.
Signed-off-by: Ian Romanick <ian.d.romanick@intel.com>
Reviewed-by: Paul Berry <stereotype441@gmail.com>
Since the separation of ir_var_function_in and ir_var_shader_in (similar
for out), this check is no longer necessary. Previously, global_scope
was the only way to tell which was which.
Signed-off-by: Ian Romanick <ian.d.romanick@intel.com>
Reviewed-by: Paul Berry <stereotype441@gmail.com>
Future patches will add some extra code to this path, and some of that
code will want to exit from the explicit location code early.
v2: Change a geometry shader "break" to a "return" so that try to apply
a bogus geometry shader location qualifier (which could cause cascading
errors). Suggested by Paul.
Signed-off-by: Ian Romanick <ian.d.romanick@intel.com>
Reviewed-by: Paul Berry <stereotype441@gmail.com>
This avoids a defect in lower_output_reads.
The problem is lower_output_reads treats the gl_FragData array as a single
variable. It first redirects all output writes to a temporary variable (array)
and then writes the whole temporary variable to the output, generating
assignments to all elements of gl_FragData.
BTW this pass can be modified to lower all arrays, not just inputs and outputs.
The question is whether it is worth it.
Reviewed-by: Paul Berry <stereotype441@gmail.com>
v2: addressed Paul Berry's comments
Almost a trivial change, it boils down to renaming a few identifiers
so their names still make sense for opaque types other than sampler.
Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
Fix the linker to deal with intrinsic functions which are undefined
all the way down to the driver back-end, and introduce intrinsic
definition helpers in the built-in generator.
We still need to figure out what kind of interface we want for drivers
to communicate to the GLSL front-end which of the supported intrinsics
should use a default GLSL implementation and which should use a
hardware-specific override. As there's no default GLSL implementation
for atomic ops, this seems like something we can worry about later on.
Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
v2: Define local helper function to generate ir_call nodes in the
builtin generator.
