Created attachment 40231 [details] a small case for it There is some error when return with a different type with the function return type, like float func(){ int a=13; return float;} [root@x-pk1 jzhao]# /GFX/Test/Piglit_ext/piglit/bin/glslparsertest glsl-conversion-function.frag pass 1.2 Failed to compile fragment shader glsl-conversion-function.frag: 0:3(10): error: `return' with wrong type int, in function `function' returning float Shader source: float function(int i) { // return with different type return i; } void main() { int i=3; float j; j=function(i); } In the spec 1.20 I find some about implicit conversions: In some situations, an expression and its type will be implicitly converted to a different type. The following table shows all allowed implicit conversions: Type of expression Can be implicitly converted to int float I tested with Libdrm: (master)2.4.22-14-g877b2ce15b80975b4dac42657bdfb0a3da833e1c Mesa: (master)dc524adee2cfd0f115800cd4ec3f8384010f154e Xserver:(server-1.9-branch)xorg-server-1.9.2-7-g46314e1e7ad05d6ff6a2f722b09a76f2931db7f5 Xf86_video_intel: (master)2.13.901-2-g3c5b1399e29ef577b8b91655b5e1c215d1b6dfbb Kernel: (drm-intel-fixes)69669455b049c0f1f04bb306625c5d4db6838b11
As is, that test case should fail. The shader must contain the line '#version 120' to use the 1.20 rules. However, adding that line does not cause the test to pass. This should be easy enough to fix. I'm reassigning this to Ken, but perhaps Chad should take it instead.
Created attachment 40275 [details] a small case for it adding version number in it
Created attachment 40296 [details] [review] Fix for bug, but causes Piglit regressions
I have attached a patch that causes the test case to pass. But, this patch causes two Piglit regressions: * glslparsertest/glsl2/constructor-28.vert * glslparsertest/glsl2/return-conversion-2.frag I am not sure if implicit conversions should be allowed on return values. If Mesa did allow such conversion, then this function definition would be legal: #version 130 vec2 f() { if (A) return 1.0; // convert to vec2(1.0, 1.0); else if (B) return false; // convert to vec2(0.0, 0.0); else return vec3(1.0, 2.0, 3.0); // convert to vec2(1.0, 2.0) } Do we really desire this behavior? Unfortunately, the GLSL spec is silent on this issue; it does not explicitly specify if return values are or are not implicitly converted. (See sections 6.1 and 4.1.10). However, the GLSL 4.10 spec contains a disclaimer statement that, under a strict interpretation, seems to ban this implicit conversion behavior: (4.1.10) "The conversions in the table above are done only as indicated by other sections of this specification."
(In reply to comment #4) > I have attached a patch that causes the test case to pass. But, this patch > causes two Piglit regressions: > * glslparsertest/glsl2/constructor-28.vert > * glslparsertest/glsl2/return-conversion-2.frag As I mentioned in comment #1, the automatic conversions should only happen in version 1.20 and later. Your patch enables the conversions universally. This is why 1.10 tests fail. Other than that, the patch looks correct. Other places in the compiler break initializers at the = instead of the (. For example: bool conversion_success = apply_implicit_conversion(state->current_function->return_type, ret, state); This /usually/ results in fewer line continuations. > I am not sure if implicit conversions should be allowed on return values. If > Mesa did allow such conversion, then this function definition would be legal: > #version 130 > vec2 f() { > if (A) > return 1.0; // convert to vec2(1.0, 1.0); > else if (B) > return false; // convert to vec2(0.0, 0.0); > else > return vec3(1.0, 2.0, 3.0); // convert to vec2(1.0, 2.0) > } > Do we really desire this behavior? > > Unfortunately, the GLSL spec is silent on this issue; it does not explicitly > specify if return values are or are not implicitly converted. (See sections 6.1 > and 4.1.10). However, the GLSL 4.10 spec contains a disclaimer statement that, > under a strict interpretation, seems to ban this implicit conversion behavior: > (4.1.10) "The conversions in the table above are done only as indicated by > other sections of this specification." The is *NOT* how implicit conversions work in GLSL. The spec is quite clear. It says that int can be implicitly converted to float. Bool is never implicitly converted. Scalar is never implicitly converted to vector. The table in section 4.1.10 (Implicit Conversions) lists all of the possible conversions. Note that the set of available conversions changes slightly between version 1.20 and 1.30. The function apply_implicit_conversion should do the right thing.
(In reply to comment #5 > The is *NOT* how implicit conversions work in GLSL. The spec is quite clear. > It says that int can be implicitly converted to float. Bool is never > implicitly converted. Scalar is never implicitly converted to vector. The > table in section 4.1.10 (Implicit Conversions) lists all of the possible > conversions. Note that the set of available conversions changes slightly > between version 1.20 and 1.30. > I was just reposting to retract that.mistaken example, but you beat me to it by nanoseconds.
Created attachment 40319 [details] [review] Fixes bug. Causes no Piglit regressions. I fixed the faulty logic in the previous patch. The new path fixes the bug without causing Piglit regressions. Ian, do you see any problems?
Well, with that patch, the code and comments are conflicting: /* foo is not allowed! */ do_foo(); I'd say remove the comment. I thought it strange at first that you're not checking the return value from apply_implicit_conversions, but then I convinced myself that apply_implicit_conversions(ta, b, state) will return true if and only if ta == b->type...so it seems fine. Also, it's unclear whether or not this behavior is legitimate or not. I tested this back in June (commit 18707eba1c), and discovered that nVidia accepts it without warning, but ATI rejects it: ERROR: 0:5: error(#247) Function return is not matching type I don't see anything saying it's disallowed, but it's not clear that it -is- allowed, either, and the GLSL specification does state that implicit conversions are disallowed except where specified. Regardless of what we decide, the first part of the patch - changing ir_expression* to ir_rvalue* - needs to happen. The only reason "return 0" works right now is due to the compiler laying out the "type" field from ir_instruction in the same location for both ir_constant and ir_expression. It's crazy.
(In reply to comment #8) > Well, with that patch, the code and comments are conflicting: > > /* foo is not allowed! */ > do_foo(); > > I'd say remove the comment. The patch does remove the comment. - /* Implicit conversions are not allowed for return values. */ > Also, it's unclear whether or not this behavior is legitimate or not. I tested So... who wants to make the call as to whether this implicit conversion is legit? I vote that it is.
(In reply to comment #9) > So... who wants to make the call as to whether this implicit conversion is > legit? I vote that it is. I'd vote yes.
(In reply to comment #8) > Also, it's unclear whether or not this behavior is legitimate or not. I tested > this back in June (commit 18707eba1c), and discovered that nVidia accepts it > without warning, but ATI rejects it: > ERROR: 0:5: error(#247) Function return is not matching type > > I don't see anything saying it's disallowed, but it's not clear that it -is- > allowed, either, and the GLSL specification does state that implicit > conversions are disallowed except where specified. Since two major vendors have differing behavior, I'm inclined to leave our implementation as-is for now. I've submitted a bug against the GLSL specification, so hopefully we'll get clarification one way or the other soon. > Regardless of what we decide, the first part of the patch - changing > ir_expression* to ir_rvalue* - needs to happen. The only reason "return 0" > works right now is due to the compiler laying out the "type" field from > ir_instruction in the same location for both ir_constant and ir_expression. > It's crazy. It lays them out in the same location because it has to: type comes from the common base class. You are right, though, the type should be changed to ir_rvalue.
(In reply to comment #11) > You are right, though, the type should be changed to > ir_rvalue. Even though we're holding off on the "bug" fix, we should still patch the erroneous typecast. I've submitted the tiny patch to the mesa-dev list.
(In reply to comment #8) > Also, it's unclear whether or not this behavior is legitimate or not. I tested > this back in June (commit 18707eba1c), and discovered that nVidia accepts it > without warning, but ATI rejects it: > ERROR: 0:5: error(#247) Function return is not matching type > > I don't see anything saying it's disallowed, but it's not clear that it -is- > allowed, either, and the GLSL specification does state that implicit > conversions are disallowed except where specified. This issues was discussed in the ARB GLSL conference call today. Implicit conversions are not performed on the values of return statements. We should leave our compiler as-is, and remove the erroneous test case.
> (In reply to comment #8) > This issues was discussed in the ARB GLSL conference call today. Implicit > conversions are not performed on the values of return statements. We should > leave our compiler as-is, and remove the erroneous test case. Erroneous test case no longer exists. Marking this bug CLOSED/NOTABUG.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.