On Sat, Dec 19, 2015 at 11:46 PM, Connor Abbott <cwabbo...@gmail.com> wrote:
> On Sat, Dec 19, 2015 at 11:08 PM, Rob Clark <robdcl...@gmail.com> wrote:
>> On Sat, Dec 19, 2015 at 10:30 PM, Connor Abbott <cwabbo...@gmail.com> wrote:
>>> On Sat, Dec 19, 2015 at 10:19 PM, Rob Clark <robdcl...@gmail.com> wrote:
>>>> On Sat, Dec 19, 2015 at 10:05 PM, Connor Abbott <cwabbo...@gmail.com>
>>>> wrote:
>>>>> On Sat, Dec 19, 2015 at 9:52 PM, Rob Clark <robdcl...@gmail.com> wrote:
>>>>>> On Sat, Dec 19, 2015 at 9:30 PM, Connor Abbott <cwabbo...@gmail.com>
>>>>>> wrote:
>>>>>>> On Sat, Dec 19, 2015 at 9:18 PM, Rob Clark <robdcl...@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.
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.
>>
>> 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 <cwabbo...@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 <robdcl...@gmail.com>
>>>>>>>>> wrote:
>>>>>>>>>> From: Rob Clark <robcl...@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 <robcl...@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@lists.freedesktop.org
>>>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
