[Mesa-dev] [PATCH] nir/builder: fix C90 build errors
Connor Abbott
cwabbott0 at gmail.com
Sat Dec 19 21:25:57 PST 2015
On Sun, Dec 20, 2015 at 12:02 AM, Rob Clark <robdclark at gmail.com> wrote:
> On Sat, Dec 19, 2015 at 11:46 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
>> 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.
>
> Well, I agree the *reason* for special rules is different, but IMO the
> end result is the same (ie. special rules).. but I think we are not
> converging here so I'll just call this a difference of opinion.
>
>>>
>>> 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.
>
> yeah, I think scons build just simply ignores $NIR_SOURCES from
> Makefile.sources, whereas Makefile.am does:
>
> libgallium_la_SOURCES = \
> $(C_SOURCES) \
> $(NIR_SOURCES) \
> $(GENERATED_SOURCES)
>
>
>> 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.
>
> see below
>
>> 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?
>
> no objection.. I started in gallium since I didn't really know how
> i965 handles some of that. I do add the y-transform lowering pass,
> for dealing w/ gl_FragCoord/fddy for coordinate origin and pixel
> center in nir.. although that choice was a bit arbitrary. Anything
> that is useful or potentially useful for i965, we can move.
Well, we already have stuff like nir_lower_two_sided_color that's not
currently used by i965 in src/glsl/nir, so I don't think anyone would
really be opposed to putting it in there. I don't think there's
anything in there that's very tgsi-to-nir (and therefore gallium)
specific (correct me if I'm wrong), so it seems like that would be a
better fit for something like this, and it would avoid the problem
you're trying to fix.
>
> But, this would still be an issue for tgsi_to_nir, which I guess
> wouldn't make sense in glsl/nir. Although turns out we solve that
> today by:
>
> #ifdef __GNUC__
> #pragma GCC diagnostic ignored "-Wdeclaration-after-statement"
> #endif
>
> which we could drop with this patch. We could ofc also stuff the same
> into nir_emulate. Tbh dropping the gcc specific pragma's and
> tolerating C90 in the nir header files seems nicer.
Although, dropping the pragma's also opens the door for more build breakage...
It seems like tgsi_to_nir already has this hack. IMHO it would be
better if you built a separate libgallium_nir.a without the
-Werror-declaration-after-statement to avoid this, but if you agree
with the above I guess that can wait.
>
>>>
>>> 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.
>
> Well, if you introduced an inline fxn foo() in the headers, but some
> .c file that #include that header already had a foo(), that would
> break. This is why I'm saying that, to be pedantic, any header file
> change should trigger a wider test recompile. And once you do that,
> you'll catch any unintended breakages that you were worrying about
> above.
>
> BR,
> -R
>
>>>
>>> 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