Re: [Mesa-dev] A few fixes for the preprocessor for GLSL 1.30
Ian Romanick i...@freedesktop.org writes: Though, it turns out that this deviates from what C99 specifies. Specifically, the ISO/IEC 9899:TC2 Committee Draft dated May 6, 2005 says: After all replacements due to macro expansion and the defined unary operator have been performed, all remaining identifiers are replaced with the pp-number 0... I've submitted a GLSL spec bug. In the mean time, I'd suggest leaving the tests and our implementation as-is. Hi Ian, Back in September, 2011, I did leave the test and implementation as-is. Then, in June 2012, I changed the behavior of our implementation, (I believe this was in response to an upcoming change in the specification you were aware of). At the time, I updated some make check tests, but failed to update piglit, (leading to bugzilla bug #51631). I'd like to fix that now. I can see by comparing the GLSL 4.30 specification to GLSL 1.30 that the following sentence has been dropped, (justifying the currently implemented behavior in Mesa): It is an error to use #if or #elif on expressions containing undefined macro names, other than as arguments to the defined operator. But I'd like a little guidance on what to do with the piglit test. We're still only support GLSL 1.30 in our implementation, right? Should we be supporting the formerly-specified behavior until we start seeing #version 430? Or should we change piglit to just check for the new behavior, even with the old version? (You mentioned earlier in this thread that AMD and nVidia binary blobs were previously implementing the new behavior anyway). -Carl -- carl.d.wo...@intel.com pgpDE4i6rwDya.pgp Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] A few fixes for the preprocessor for GLSL 1.30
On 02/22/2013 01:14 PM, Carl Worth wrote: Ian Romanick i...@freedesktop.org writes: Though, it turns out that this deviates from what C99 specifies. Specifically, the ISO/IEC 9899:TC2 Committee Draft dated May 6, 2005 says: After all replacements due to macro expansion and the defined unary operator have been performed, all remaining identifiers are replaced with the pp-number 0... I've submitted a GLSL spec bug. In the mean time, I'd suggest leaving the tests and our implementation as-is. Hi Ian, Back in September, 2011, I did leave the test and implementation as-is. Then, in June 2012, I changed the behavior of our implementation, (I believe this was in response to an upcoming change in the specification you were aware of). At the time, I updated some make check tests, but failed to update piglit, (leading to bugzilla bug #51631). I'd like to fix that now. I can see by comparing the GLSL 4.30 specification to GLSL 1.30 that the following sentence has been dropped, (justifying the currently implemented behavior in Mesa): It is an error to use #if or #elif on expressions containing undefined macro names, other than as arguments to the defined operator. But I'd like a little guidance on what to do with the piglit test. We're still only support GLSL 1.30 in our implementation, right? Should we be supporting the formerly-specified behavior until we start seeing #version 430? Or should we change piglit to just check for the new behavior, even with the old version? (You mentioned earlier in this thread that AMD and nVidia binary blobs were previously implementing the new behavior anyway). -Carl I'd recommend assuming the new behavior in all language versions, with a comment in the test indicating that both AMD and nVidia's drivers have this behavior, so it's the de facto standard. My $0.02 anyway. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] A few fixes for the preprocessor for GLSL 1.30
Eric recentl mentioned to me that when setting the following
environment variables with current master mesa:
MESA_GL_VERSION_OVERRIDE=3.0
MESA_GLSL_VERSION_OVERRIDE=130
that 8 piglit preprocessor tests start to fail.
I've investigated all of these failures and am here attaching a patch
series for review that fixes 5 of them. In the testing I've done, I
don't see any changes from this patch series other than the desired
piglit fixes. But, of course, I'd be happy for any review.
For the remaining three failures, I believe the tests themselves are
broken and should be fixed. These fall into two case:
1. feature-macros/gl_gragment_precision_high.{frag,vert}
In these tests, the actual preprocessor functionality being tested,
(that a directive of #version 130 causes the
GL_FRAGMENT_PRECISION_HIGH macro to be pre-defined), is working fine.
The tests are failing due to the following code:
float f() { return 0; }
Our implementation does not currently do implicit promotion here,
so compilation fails with a type error:
error: `return' with wrong type int, in function `f' returning
float
The GLSL specification is not explicit on whether this promotion
should be happening, but other implementations appear to do it. So
perhaps the promotion should be added to our implementation.
Meanwhile, the test could legitimately be fixed to remove the type
error which is a side issue from what the test is actually trying
to do.
2. if/if-arg-must-be-defined-02.frag
This test is trying to elicit an undefined macro error with
something like the following code:
#if 1
...
#elif UNDEFINED_MACRO
...
#endif
I propose changing the test to use #if 0 instead, in which case
the current implementation would pass. It's not actually desirable
to generate an error in the above case. Consider a use of a
possibly undefined macro:
#if PERHAPS_UNDEFINED 5
...
#endif
In order to protect this use, the user might add something like the
following (or similar with #ifndef):
#if ! defined PERHAPS_UNDEFINED
... deal with it ...
#elif PERHAPS_UNDEFINED
...
#endif
In this case, the user's expectation is to get no error with this
code. Yet this is precisly the formulation the current test case
uses to elicit the error.
I'll be glad to push changes to piglit to fix these cases unless
someone can argue aginst that.
Thanks,
-Carl
--
carl.d.wo...@intel.com
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] A few fixes for the preprocessor for GLSL 1.30
On 09/29/2011 11:00 PM, Carl Worth wrote:
Eric recentl mentioned to me that when setting the following
environment variables with current master mesa:
MESA_GL_VERSION_OVERRIDE=3.0
MESA_GLSL_VERSION_OVERRIDE=130
that 8 piglit preprocessor tests start to fail.
I've investigated all of these failures and am here attaching a patch
series for review that fixes 5 of them. In the testing I've done, I
don't see any changes from this patch series other than the desired
piglit fixes. But, of course, I'd be happy for any review.
For the series:
Reviewed-by: Kenneth Graunke kenn...@whitecape.org
Thanks for fixing these, Carl!
For the remaining three failures, I believe the tests themselves are
broken and should be fixed. These fall into two case:
1. feature-macros/gl_gragment_precision_high.{frag,vert}
In these tests, the actual preprocessor functionality being tested,
(that a directive of #version 130 causes the
GL_FRAGMENT_PRECISION_HIGH macro to be pre-defined), is working fine.
The tests are failing due to the following code:
float f() { return 0; }
Our implementation does not currently do implicit promotion here,
so compilation fails with a type error:
error: `return' with wrong type int, in function `f' returning
float
The GLSL specification is not explicit on whether this promotion
should be happening, but other implementations appear to do it. So
perhaps the promotion should be added to our implementation.
Odd. I could've sworn that was allowed (it would make sense), but
you're right: the spec never explicitly allows that, so strictly, it's
forbidden.
GLSL 4.20 and the GL_ARB_shading_language_420pack extension explicitly
add this functionality to the shading language, so we can add it then.
I guess we could technically add GL_ARB_shading_language_420pack support
now, since it only requires GLSL 1.30...it's essentially an extension
that backports some new front-end functionality and clarifications to
older versions of GLSL.
In any case, I'm in favor of changing the test to 0.0 instead of 0.
Meanwhile, the test could legitimately be fixed to remove the type
error which is a side issue from what the test is actually trying
to do.
2. if/if-arg-must-be-defined-02.frag
This test is trying to elicit an undefined macro error with
something like the following code:
#if 1
...
#elif UNDEFINED_MACRO
...
#endif
I propose changing the test to use #if 0 instead, in which case
the current implementation would pass. It's not actually desirable
to generate an error in the above case. Consider a use of a
possibly undefined macro:
I agree with your analysis here. I am in favor of changing the test to
#if 0, as you suggest.
#if PERHAPS_UNDEFINED 5
...
#endif
In order to protect this use, the user might add something like the
following (or similar with #ifndef):
#if ! defined PERHAPS_UNDEFINED
... deal with it ...
#elif PERHAPS_UNDEFINED
...
#endif
In this case, the user's expectation is to get no error with this
code. Yet this is precisly the formulation the current test case
uses to elicit the error.
I'll be glad to push changes to piglit to fix these cases unless
someone can argue aginst that.
Nope, sounds good to me. Thanks!
Thanks,
-Carl
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] A few fixes for the preprocessor for GLSL 1.30
On Thu, 29 Sep 2011 23:43:49 -0700, Kenneth Graunke kenn...@whitecape.org wrote: For the series: Reviewed-by: Kenneth Graunke kenn...@whitecape.org Thanks for fixing these, Carl! No problem. Thanks to you (and Eric) for the review. I'll be glad to push changes to piglit to fix these cases unless someone can argue aginst that. Nope, sounds good to me. Thanks! OK. I pushed the mesa changes seen here, and the obvious piglit patches as discussed. Do please continue to let me know if the preprocessor goes wrong again in the future. -Carl -- carl.d.wo...@intel.com pgp08wF41cVS1.pgp Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] A few fixes for the preprocessor for GLSL 1.30
On 09/29/2011 11:00 PM, Carl Worth wrote:
Eric recentl mentioned to me that when setting the following
environment variables with current master mesa:
MESA_GL_VERSION_OVERRIDE=3.0
MESA_GLSL_VERSION_OVERRIDE=130
that 8 piglit preprocessor tests start to fail.
I've investigated all of these failures and am here attaching a patch
series for review that fixes 5 of them. In the testing I've done, I
don't see any changes from this patch series other than the desired
piglit fixes. But, of course, I'd be happy for any review.
For the remaining three failures, I believe the tests themselves are
broken and should be fixed. These fall into two case:
1. feature-macros/gl_gragment_precision_high.{frag,vert}
In these tests, the actual preprocessor functionality being tested,
(that a directive of #version 130 causes the
GL_FRAGMENT_PRECISION_HIGH macro to be pre-defined), is working fine.
The tests are failing due to the following code:
float f() { return 0; }
Our implementation does not currently do implicit promotion here,
so compilation fails with a type error:
error: `return' with wrong type int, in function `f' returning
float
The GLSL specification is not explicit on whether this promotion
should be happening, but other implementations appear to do it. So
perhaps the promotion should be added to our implementation.
It is not allowed until a much later GLSL version.
Meanwhile, the test could legitimately be fixed to remove the type
error which is a side issue from what the test is actually trying
to do.
The test should be fixed to return 0.0.
2. if/if-arg-must-be-defined-02.frag
Neither this test nor if-arg-must-be-defined-01 produce the desired
result on NVIDIA's blob or AMD's blob. I'd be curious to know what it
does on Apple's implementation.
In any case, the test quotes specific language from the GLSL spec that
explicitly forbids this behavior, so I'm somewhat reluctant to deviate
from the spec'ed behavior.
This test is trying to elicit an undefined macro error with
something like the following code:
#if 1
...
#elif UNDEFINED_MACRO
...
#endif
I propose changing the test to use #if 0 instead, in which case
the current implementation would pass. It's not actually desirable
to generate an error in the above case. Consider a use of a
possibly undefined macro:
#if PERHAPS_UNDEFINED 5
...
#endif
In order to protect this use, the user might add something like the
following (or similar with #ifndef):
#if ! defined PERHAPS_UNDEFINED
... deal with it ...
#elif PERHAPS_UNDEFINED
...
#endif
In this case, the user's expectation is to get no error with this
code. Yet this is precisly the formulation the current test case
uses to elicit the error.
I'll be glad to push changes to piglit to fix these cases unless
someone can argue aginst that.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] A few fixes for the preprocessor for GLSL 1.30
On 09/30/2011 12:18 PM, Ian Romanick wrote: On 09/29/2011 11:00 PM, Carl Worth wrote: 2. if/if-arg-must-be-defined-02.frag Neither this test nor if-arg-must-be-defined-01 produce the desired result on NVIDIA's blob or AMD's blob. I'd be curious to know what it does on Apple's implementation. In any case, the test quotes specific language from the GLSL spec that explicitly forbids this behavior, so I'm somewhat reluctant to deviate from the spec'ed behavior. Though, it turns out that this deviates from what C99 specifies. Specifically, the ISO/IEC 9899:TC2 Committee Draft dated May 6, 2005 says: After all replacements due to macro expansion and the defined unary operator have been performed, all remaining identifiers are replaced with the pp-number 0... I've submitted a GLSL spec bug. In the mean time, I'd suggest leaving the tests and our implementation as-is. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] A few fixes for the preprocessor for GLSL 1.30
On Fri, 30 Sep 2011 12:32:13 -0700, Ian Romanick i...@freedesktop.org wrote: In any case, the test quotes specific language from the GLSL spec that explicitly forbids this behavior, so I'm somewhat reluctant to deviate from the spec'ed behavior. We're not deviating from what is specified. We do raise an error when an undefined macro is used within an #if or #elif (other than as an argument to defined). The only difference between our implementation and the original test is that is that the test expected us to flag this error when the condition was irrelevant, such as this: #if 0 ... #elif UNDEFINED_MACRO ... #endif And that is not actually explicitly called out by the spec. Though, it turns out that this deviates from what C99 specifies. Specifically, the ISO/IEC 9899:TC2 Committee Draft dated May 6, 2005 says: After all replacements due to macro expansion and the defined unary operator have been performed, all remaining identifiers are replaced with the pp-number 0... Yes, I noticed this was different when I did the original implementation. It wasn't hard to do what the GLSL specification says. I've submitted a GLSL spec bug. OK. If GLSL should align better with C99 then that's a good plan. -Carl pgpMt0VJkoTqe.pgp Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
