[Mesa-dev] [PATCH] nir/builder: fix C90 build errors
Rob Clark
robdclark at gmail.com
Sat Dec 19 20:08:01 PST 2015
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".
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.
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.
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