[Mesa-dev] [PATCH] nir/builder: fix C90 build errors
Connor Abbott
cwabbott0 at gmail.com
Sat Dec 19 19:05:29 PST 2015
On Sat, Dec 19, 2015 at 9:52 PM, Rob Clark <robdclark at gmail.com> wrote:
> On Sat, Dec 19, 2015 at 9:30 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
>> On Sat, Dec 19, 2015 at 9:18 PM, Rob Clark <robdclark at gmail.com> wrote:
>>> Note that this is *only* about the header files.. not the src files.
>>> I'm not proposing to make NIR support C90.
>>
>> Why would you need to only make the header filef C90 compliant? If you
>> just need to pass around a nir_shader * or something, you can just use
>> a forward declaration.
>
> turns out we happen to want structs, fxn prototypes, etc.. ie. what
> happens to be in the header ;-)
>
> I guess there is possibly a way to make it so that we build certain
> files w/ different warning flags or something (but dropping -Werror
> seems like the wrong thing). At any rate, that sounds harder than a
> trivial few line patch. And as long as it is trivial to keep the nir
> headers C90-clean, that seems like the better thing to do. I mean, we
> already take some precautions in the header files to keep them usable
> from c++..
Yes, we make it C++-safe because a few parts of core NIR are written
in C++ (glsl-to-nir) as well as i965. On the other hand, we've always
allowed declarations mixed with code in NIR since day one, and making
the requirements on the header different from the requirements on
everything else is confusing and inconsistent.
>
>>>
>>> I will at some point, before it is ready to merge, need to arrange the
>>> NIR related bits in mesa st so that we can build without it, for
>>> benefit of the MSVC folks.
>>>
>>> (It might be useful someday to use NIR more extensively in mesa st,
>>> and use nir->tgsi pass, so we can do all the opt passes in NIR..
>>> although that is *way* more ambitious than what I want to do right
>>> now. With any luck, by the time we get to that point, we can rely on
>>> a less braindead version of MSVC?)
>>
>> My understanding is that mesa/st doesn't need to be built with old
>> MSVC, just gallium.
>
> Oh, ok, if that is in fact the case, it simplifies some things. Tbh
> I'm not 100% clear on what parts are used on windows..
>
> But like I said, if it is just trivial things to keep the nir headers
> C90-clean, then why not just do that? I mean, if you have something
> planned that would actually make it a burden to keep the headers
> C90-clean we can revisit, but I can't really imagine what that would
> be. Usually the static-inline stuff tends to be relatively simple
> stuff (since you aren't wanting to duplicate a lot of complex stuff in
> many object files).
Just because it's trivial to fix doesn't mean that it won't break in
the future. Now, the build is going to break every time someone adds
something to nir.h that happens to use some C99 features and doesn't
test gallium, all because a few files have an unnecessary extra
-Werror. Let's just fix the root problem instead.
>
> BR,
> -R
>
>>>
>>> On Sat, Dec 19, 2015 at 8:58 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
>>>> We haven't allowed NIR in core gallium before, since core gallium has
>>>> to be built with some old version of MSVC that doesn't support many
>>>> C99 features that we really wanted to use. The only reason that
>>>> -Werror exists is for compatibility with old MSVC, and if you want to
>>>> use NIR with something that needs to build with old MSVC, there are
>>>> going to be much bigger changes needed, and we'd rather avoid that. If
>>>> you just want to add some NIR-specific stuff that e.g. softpipe
>>>> doesn't need to compile against, then you should fix the build system
>>>> not to add the warning.
>>>>
>>>> On Sat, Dec 19, 2015 at 5:39 PM, Rob Clark <robdclark at gmail.com> wrote:
>>>>> From: Rob Clark <robclark at freedesktop.org>
>>>>>
>>>>> We are going to start using nir_builder.h from some gallium code, which
>>>>> is currently only C90. Which results in:
>>>>>
>>>>> In file included from nir/nir_emulate.c:26:0:
>>>>> ../../../src/glsl/nir/nir_builder.h: In function ‘nir_build_alu’:
>>>>> ../../../src/glsl/nir/nir_builder.h:132:4: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
>>>>> unsigned num_components = op_info->output_size;
>>>>> ^
>>>>> In file included from nir/nir_emulate.c:26:0:
>>>>> ../../../src/glsl/nir/nir_builder.h: In function ‘nir_ssa_for_src’:
>>>>> ../../../src/glsl/nir/nir_builder.h:271:4: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
>>>>> nir_alu_src alu = { NIR_SRC_INIT };
>>>>> ^
>>>>> cc1: some warnings being treated as errors
>>>>>
>>>>> Signed-off-by: Rob Clark <robclark at freedesktop.org>
>>>>> ---
>>>>> Not sure if I should just go ahead and push this sort of thing. Or
>>>>> if we can start requiring C99 for gallium?
>>>>>
>>>>> src/glsl/nir/nir_builder.h | 7 +++++--
>>>>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/src/glsl/nir/nir_builder.h b/src/glsl/nir/nir_builder.h
>>>>> index 332bb02..6f30306 100644
>>>>> --- a/src/glsl/nir/nir_builder.h
>>>>> +++ b/src/glsl/nir/nir_builder.h
>>>>> @@ -115,6 +115,8 @@ nir_build_alu(nir_builder *build, nir_op op, nir_ssa_def *src0,
>>>>> {
>>>>> const nir_op_info *op_info = &nir_op_infos[op];
>>>>> nir_alu_instr *instr = nir_alu_instr_create(build->shader, op);
>>>>> + unsigned num_components;
>>>>> +
>>>>> if (!instr)
>>>>> return NULL;
>>>>>
>>>>> @@ -129,7 +131,7 @@ nir_build_alu(nir_builder *build, nir_op op, nir_ssa_def *src0,
>>>>> /* Guess the number of components the destination temporary should have
>>>>> * based on our input sizes, if it's not fixed for the op.
>>>>> */
>>>>> - unsigned num_components = op_info->output_size;
>>>>> + num_components = op_info->output_size;
>>>>> if (num_components == 0) {
>>>>> for (unsigned i = 0; i < op_info->num_inputs; i++) {
>>>>> if (op_info->input_sizes[i] == 0)
>>>>> @@ -265,10 +267,11 @@ nir_channel(nir_builder *b, nir_ssa_def *def, unsigned c)
>>>>> static inline nir_ssa_def *
>>>>> nir_ssa_for_src(nir_builder *build, nir_src src, int num_components)
>>>>> {
>>>>> + nir_alu_src alu = { NIR_SRC_INIT };
>>>>> +
>>>>> if (src.is_ssa && src.ssa->num_components == num_components)
>>>>> return src.ssa;
>>>>>
>>>>> - nir_alu_src alu = { NIR_SRC_INIT };
>>>>> alu.src = src;
>>>>> for (int j = 0; j < 4; j++)
>>>>> alu.swizzle[j] = j;
>>>>> --
>>>>> 2.5.0
>>>>>
>>>>> _______________________________________________
>>>>> mesa-dev mailing list
>>>>> mesa-dev at lists.freedesktop.org
>>>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list