[Mesa-dev] [PATCH] nir/builder: fix C90 build errors
Connor Abbott
cwabbott0 at gmail.com
Sat Dec 19 20:46:19 PST 2015
On Sat, Dec 19, 2015 at 11:08 PM, Rob Clark <robdclark at gmail.com> wrote:
> On Sat, Dec 19, 2015 at 10:30 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
>> On Sat, Dec 19, 2015 at 10:19 PM, Rob Clark <robdclark at gmail.com> wrote:
>>> On Sat, Dec 19, 2015 at 10:05 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
>>>> 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.
>>>
>>> But we already have "confusing and inconsistent" (in quotes, since I
>>> don't think it is nearly as bad as you are making it out to be) thanks
>>> to the C++ requirement..
>>
>> No, it's not. NIR contains both C++ and C99 code. Now, you're saying
>> you want a few things to be C90, despite the fact that the C90 parts
>> are useless without the C99 parts, so you obviously already have a
>> C99-capable compiler... why?
>
> My point is, it's a header file for C (c99) code, yet there are some
> special rules thanks to C++. IMO that is just as much (again in
> quotes) "confusing and inconsistent".
No, having support for C++ and C is much less confusing than having to
support different versions of C. The former is required, but the
latter is simply due to a resistance to fixing the build system.
>
> The nir_emulate (and tgsi_to_nir) code is in gallium, although not
> built from scons build files. Dropping the C90 restriction would, it
> seems, require splitting that up and re-arranging the build, which is
> (a) harder, and (b) likely to encounter just as much or more pointless
> BS bikeshedding.
How does that work? Does scons have a different list of things to
build from the Autotools build? In any case, punishing code not built
by scons by forcing it to use C90 without the gnu extension for
declarations mixed with code seems wrong.
I'm not worried about the size of the changes -- they're small enough
-- but what I am worried about is the very likely possibility that
this causes unnecessary pain for both you and me in the future when
someone unexpectedly breaks the Gallium build due to what should be
trivial changes in nir.h or files that it includes.
Also, I had a look at what you wanted to do (i.e. nir_emulate), and it
seems pretty driver-agnostic... any reason not to put this in
src/glsl/nir and be done with it?
>
> I guess if there are some pragmas we could put in the code to say "oh,
> no, I really meant c99", maybe that would work.. although I guess that
> would be equally unacceptable to someone else..
>
>>>
>>>>>
>>>>>>>
>>>>>>> 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.
>>>
>>> -Werror is not the problem.. maybe lack of --std=c99 is.. but defn not -Werror
>>>
>>> Are there *really* that many people making changes to nir, who are
>>> building mesa without gallium? I think we've already convinced most
>>> people making changes on NIR that they should be building
>>> vc4/freedreno (which requires building gallium) to avoid the sort of
>>> breakage that has already happened a couple times. And if they
>>> aren't, they should be. And since this is all compile-time issues,
>>> rather than runtime, I kind of think you are making a mountain out of
>>> a mole-hill here..
>>
>> We've convinced people to check *when their changes might affect those
>> drivers*. In the past, merely adding an inline function to a NIR
>> header didn't qualify for that, since we managed to keep the build
>> requirements relatively consistent between gallium and non-gallium...
>> for now. Again, why don't you just fix the build system?
>
> To be completely pedantic, adding a inline fxn in a header could
> actually break things. So any touching headers should rebuild other
> drivers, gallium, etc.
No, it couldn't break things, or at least it couldn't break the build,
assuming that Gallium and non-Gallium use the same build flags...
which is exactly what's causing problems here.
>
> BR,
> -R
>
>>>
>>> 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