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..
>>
>>>>
>>>> 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..
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