Re: [Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering
On 08/28/2018 12:40 PM, Ian Romanick wrote: > On 08/28/2018 12:09 PM, Rogovin, Kevin wrote: >> Hi, >> >>> Is that tested? >> >> I have tested it in a simple shader test I made (i.e. not in a dedicated >> test suite such as dEQP, piglit or something else). The created assembly is >> identical. The specific example is a shader where I replace calls of >> beginFragmentShaderOrderingINTEL() with beginInvocationInterlockARB() and >> the created assembly is the same. Running with INTEL_DEBUG=fs produced >> identical output except the SSA NIR had the different opcode. AFAIK, the >> current Mesa implementation of the ARB extension does not in any way shape >> or form enforce any of "no control flow", "only once and only in main" >> requirements. Indeed, Mesa happily accepts code even without the >> endInvocationInterlockARB() call as well. Personally, I am happy that it is >> more accepting rather than rejecting the code. > > That's actually terrible and needs to be fixed ASAP. That's how we end > up with the "it works on " rubbish that has basically > ruined GLSL. Test cases sent to the piglit list. >> -Kevin >> ___ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev >> > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering
On Tue, Aug 28, 2018 at 4:13 PM Rogovin, Kevin wrote: > Hi, > > > You are completely missing the point. Any test which tests the > > GL_ARB_fragment_shader_interlock extension with a > > beginInvocationInterlockARB() call inside of control-flow would be an > > invalid test for GL_ARB_fragment_shader_interlock since that behavior is > > undefined. Therefore, any such test would be a bad test. Unless we have > > tests which test beginFragmentShaderOrderingINTEL() inside non-uniform > > control-flow, it is insufficiently tested. Therefore, any tests we may > > have for GL_ARB_fragment_shader_interlock are either invalid use of the > ARB > > extension or they are insufficient to test the INTEL extension. > > My apologies, I misunderstood your question "is that tested?"; I thought > the question was about if the assentation that the current implementation > of beginFragmentShaderOrderingINTEL() and beginInvocationInterlockARB() > were functionally interchangeable has been verified/tested. > > FWIW, a test where beginInvocationInterlockARB() is called within any > control flow or for that matter not called from main(), the shader is > supposed to be rejected according the spec. In a negative way, it is > another test. Though, while acknowledging Ian's point that "works for > vendor X" has caused a great deal of GLSL fragmentation, I still personally > prefer to implementation to try to take what applications throw at them as > much as reasonable possible. However, that is not my decision to make and > whatever the community chooses is what it shall be. > If the spec says to reject it, then we should reject it and we should have tests that ensure that we do so. That's not really a matter of "take what applications throw at them as much as reasonable possible", it's a matter of implementing a spec. The moment people start straying from the spec because they think their implementation is more reasonable than what's spec'd, it's no longer a spec. In this case, that's exactly why the INTEL extension exists. > I agree with you that taking the current ARB test and then adding some > non-uniform flow control (along with removing the layout() qualifiers) to > it is the best way to go and I *think* Plamena has offered to do so. > > At this point, I can just let for it all to sort out and bow out of the > discussion at this point as the series seems to have brought additional > issues up that are out-of-scope for what I had in mind with the series > (namely something small-simple to expose the extension and not enforcing > the limitation of the ARB extension). > One of my fears here is whether or not the compiler will actually treat the instruction as a barrier like it's supposed to. We don't do a lot with respect to optimization around barriers right now so it's probably ok. However, the two extensions are sufficiently different from an API surface perspective (even if the implementation is identical!) that we need to be careful about making sure they're both tested. --Jason ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering
Hi, > You are completely missing the point. Any test which tests the > GL_ARB_fragment_shader_interlock extension with a > beginInvocationInterlockARB() call inside of control-flow would be an > invalid test for GL_ARB_fragment_shader_interlock since that behavior is > undefined. Therefore, any such test would be a bad test. Unless we have > tests which test beginFragmentShaderOrderingINTEL() inside non-uniform > control-flow, it is insufficiently tested. Therefore, any tests we may > have for GL_ARB_fragment_shader_interlock are either invalid use of the ARB > extension or they are insufficient to test the INTEL extension. My apologies, I misunderstood your question "is that tested?"; I thought the question was about if the assentation that the current implementation of beginFragmentShaderOrderingINTEL() and beginInvocationInterlockARB() were functionally interchangeable has been verified/tested. FWIW, a test where beginInvocationInterlockARB() is called within any control flow or for that matter not called from main(), the shader is supposed to be rejected according the spec. In a negative way, it is another test. Though, while acknowledging Ian's point that "works for vendor X" has caused a great deal of GLSL fragmentation, I still personally prefer to implementation to try to take what applications throw at them as much as reasonable possible. However, that is not my decision to make and whatever the community chooses is what it shall be. I agree with you that taking the current ARB test and then adding some non-uniform flow control (along with removing the layout() qualifiers) to it is the best way to go and I *think* Plamena has offered to do so. At this point, I can just let for it all to sort out and bow out of the discussion at this point as the series seems to have brought additional issues up that are out-of-scope for what I had in mind with the series (namely something small-simple to expose the extension and not enforcing the limitation of the ARB extension). -Kevin -Original Message- From: Rogovin, Kevin Sent: Tuesday, August 28, 2018 10:10 PM To: 'mesa-dev@lists.freedesktop.org' Cc: ja...@jlekstrand.net Subject: [Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering Hi, > Is that tested? I have tested it in a simple shader test I made (i.e. not in a dedicated test suite such as dEQP, piglit or something else). The created assembly is identical. The specific example is a shader where I replace calls of beginFragmentShaderOrderingINTEL() with beginInvocationInterlockARB() and the created assembly is the same. Running with INTEL_DEBUG=fs produced identical output except the SSA NIR had the different opcode. AFAIK, the current Mesa implementation of the ARB extension does not in any way shape or form enforce any of "no control flow", "only once and only in main" requirements. Indeed, Mesa happily accepts code even without the endInvocationInterlockARB() call as well. Personally, I am happy that it is more accepting rather than rejecting the code. -Kevin ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering
Kevin, Your email messages are missing the in-reply-to header, making this thread completely incomprehensible on the mailing list. Please fix your mail client to include the standard headers so we can follow what you are saying. -Mark "Rogovin, Kevin" writes: > Hi, > >> Having the same underlying compiler intrinsic and having the same behavior >> are not the same thing. The INTEL extension allows strictly more >> functionality than the ARB extension so it needs even more testing. In >> particular, it allows you to do the barrier in non-uniform control-flow >> which is a very different thing than at the top of a shader like is allowed >> by ARB_fragment_shader_interlock. I expect the INTEL extension to need >> substantially more testing than the ARB one. > > Just an FYI: the Mesa implementation for the ARB (and thus NV) > extension accept shaders where the begin function takes place inside > of control flow actually. > > -Kevin > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering
On 08/28/2018 12:09 PM, Rogovin, Kevin wrote: > Hi, > >> Is that tested? > > I have tested it in a simple shader test I made (i.e. not in a dedicated test > suite such as dEQP, piglit or something else). The created assembly is > identical. The specific example is a shader where I replace calls of > beginFragmentShaderOrderingINTEL() with beginInvocationInterlockARB() and the > created assembly is the same. Running with INTEL_DEBUG=fs produced identical > output except the SSA NIR had the different opcode. AFAIK, the current Mesa > implementation of the ARB extension does not in any way shape or form enforce > any of "no control flow", "only once and only in main" requirements. Indeed, > Mesa happily accepts code even without the endInvocationInterlockARB() call > as well. Personally, I am happy that it is more accepting rather than > rejecting the code. That's actually terrible and needs to be fixed ASAP. That's how we end up with the "it works on " rubbish that has basically ruined GLSL. > -Kevin > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering
On Tue, Aug 28, 2018 at 2:10 PM Rogovin, Kevin wrote: > Hi, > > > Is that tested? > > I have tested it in a simple shader test I made (i.e. not in a dedicated > test suite such as dEQP, piglit or something else). The created assembly is > identical. The specific example is a shader where I replace calls of > beginFragmentShaderOrderingINTEL() with beginInvocationInterlockARB() and > the created assembly is the same. Running with INTEL_DEBUG=fs produced > identical output except the SSA NIR had the different opcode. AFAIK, the > current Mesa implementation of the ARB extension does not in any way shape > or form enforce any of "no control flow", "only once and only in main" > requirements. Indeed, Mesa happily accepts code even without the > endInvocationInterlockARB() call as well. Personally, I am happy that it > is more accepting rather than rejecting the code. > You are completely missing the point. Any test which tests the GL_ARB_fragment_shader_interlock extension with a beginInvocationInterlockARB() call inside of control-flow would be an invalid test for GL_ARB_fragment_shader_interlock since that behavior is undefined. Therefore, any such test would be a bad test. Unless we have tests which test beginFragmentShaderOrderingINTEL() inside non-uniform control-flow, it is insufficiently tested. Therefore, any tests we may have for GL_ARB_fragment_shader_interlock are either invalid use of the ARB extension or they are insufficient to test the INTEL extension. --Jason ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering
Hi, > Is that tested? I have tested it in a simple shader test I made (i.e. not in a dedicated test suite such as dEQP, piglit or something else). The created assembly is identical. The specific example is a shader where I replace calls of beginFragmentShaderOrderingINTEL() with beginInvocationInterlockARB() and the created assembly is the same. Running with INTEL_DEBUG=fs produced identical output except the SSA NIR had the different opcode. AFAIK, the current Mesa implementation of the ARB extension does not in any way shape or form enforce any of "no control flow", "only once and only in main" requirements. Indeed, Mesa happily accepts code even without the endInvocationInterlockARB() call as well. Personally, I am happy that it is more accepting rather than rejecting the code. -Kevin ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering
On Tue, Aug 28, 2018 at 1:23 PM Rogovin, Kevin wrote: > Hi, > > > Having the same underlying compiler intrinsic and having the same > behavior > > are not the same thing. The INTEL extension allows strictly more > > functionality than the ARB extension so it needs even more testing. In > > particular, it allows you to do the barrier in non-uniform control-flow > > which is a very different thing than at the top of a shader like is > allowed > > by ARB_fragment_shader_interlock. I expect the INTEL extension to need > > substantially more testing than the ARB one. > > Just an FYI: the Mesa implementation for the ARB (and thus NV) extension > accept shaders where the begin function takes place inside of control flow > actually. > Is that tested? ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering
Hi, > Having the same underlying compiler intrinsic and having the same behavior > are not the same thing. The INTEL extension allows strictly more > functionality than the ARB extension so it needs even more testing. In > particular, it allows you to do the barrier in non-uniform control-flow > which is a very different thing than at the top of a shader like is allowed > by ARB_fragment_shader_interlock. I expect the INTEL extension to need > substantially more testing than the ARB one. Just an FYI: the Mesa implementation for the ARB (and thus NV) extension accept shaders where the begin function takes place inside of control flow actually. -Kevin ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering
"Works on an app" is not and never has been an acceptable testing plan for landing a new feature in Mesa. There need to be tests in piglit, dEQP, or one of the conformance suites. --Jason On Tue, Aug 28, 2018 at 11:15 AM Rogovin, Kevin wrote: > Hi, > > On the questions of tests: If people want, I can adapt the test in piglit > for ARB_fragment_shader_interlock to this INTEL one. In general, I have an > app/library that uses the extension and testing of that does definitely > work on i965 (which should be utterly unsurprising since the produced > assembly is identical). Indeed the current implementation in Mesa of > ARB_fragment_shader_interlock is as flexible as the INTEL one since the > compiler accepts code with the begin/end interlock anywhere since the only > backend that supports it, i965, can easily accept it. I view the > INTEL_fragment_shader_ordering implementation in a similar light as the > NV_fragment_shader_interlock where it actually does not really do anything > new, but signals to an application that the Mesa/i965 implementation allows > more (and does it correctly) than the ARB/NV extensions restrict to. > > -Kevin > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering
On Tue, Aug 28, 2018 at 11:22 AM Manolova, Plamena < plamena.manol...@intel.com> wrote: > Hi Mark, > AFAIK there is no piglit test for this specific extension. However > underneath the hood it reuses the > functionality of ARB_fragment_shader_interlock, which has a test. I > believe the only major difference between > the two extensions is that unlike beginInvocationInterlockARB, > beginFragmentShaderOrderingINTEL > can > be called from functions other than main(). If necessary it would be > pretty straightforward to reuse most of the code > for the ARB_fragment_shader_interlock test to create one > for INTEL_fragment_shader_ordering. > Having the same underlying compiler intrinsic and having the same behavior are not the same thing. The INTEL extension allows strictly more functionality than the ARB extension so it needs even more testing. In particular, it allows you to do the barrier in non-uniform control-flow which is a very different thing than at the top of a shader like is allowed by ARB_fragment_shader_interlock. I expect the INTEL extension to need substantially more testing than the ARB one. --Jason > Thank you, > Pam > > On Tue, Aug 28, 2018 at 6:41 PM Mark Janes wrote: > >> Is there a piglit test that verifies that this feature works properly? >> >> writes: >> >> > From: Kevin Rogovin >> > >> > INTEL_fragment_shader_ordering provides the ability for shaders >> > to issue a call to gaurnantee memory write operation ordering >> > of overlapping pixels or samples. In contrast to >> > ARB_fragment_shader_interlock, INTEL_fragment_shader_ordering >> > instead of defining a critical region (which must be in main() and >> > under no flow control) provides a single function that acts like >> > a memory barrier that can be called under any control flow. >> > >> > Kevin Rogovin (2): >> > mesa: Add GL/GLSL plumbing for INTEL_fragment_shader_ordering. >> > i965: Add INTEL_fragment_shader_ordering support. >> > >> > docs/relnotes/18.3.0.html| 1 + >> > src/compiler/glsl/builtin_functions.cpp | 17 + >> > src/compiler/glsl/glsl_parser_extras.cpp | 1 + >> > src/compiler/glsl/glsl_parser_extras.h | 2 ++ >> > src/compiler/glsl/glsl_to_nir.cpp| 6 ++ >> > src/compiler/glsl/ir.h | 1 + >> > src/compiler/nir/nir_intrinsics.py | 1 + >> > src/intel/compiler/brw_fs_nir.cpp| 1 + >> > src/mesa/drivers/dri/i965/intel_extensions.c | 1 + >> > src/mesa/main/extensions_table.h | 1 + >> > src/mesa/main/mtypes.h | 1 + >> > 11 files changed, 33 insertions(+) >> > >> > -- >> > 2.17.1 >> > >> > ___ >> > mesa-dev mailing list >> > mesa-dev@lists.freedesktop.org >> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev >> > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering
"Manolova, Plamena" writes: > Hi Mark, > AFAIK there is no piglit test for this specific extension. However > underneath the hood it reuses the functionality of > ARB_fragment_shader_interlock, which has a test. I believe the only > major difference between the two extensions is that unlike > beginInvocationInterlockARB, beginFragmentShaderOrderingINTEL can be > called from functions other than main(). If necessary it would be > pretty straightforward to reuse most of the code for the > ARB_fragment_shader_interlock test to create one for > INTEL_fragment_shader_ordering. Sounds like a good plan! > Thank you, > Pam > > On Tue, Aug 28, 2018 at 6:41 PM Mark Janes wrote: > >> Is there a piglit test that verifies that this feature works properly? >> >> writes: >> >> > From: Kevin Rogovin >> > >> > INTEL_fragment_shader_ordering provides the ability for shaders >> > to issue a call to gaurnantee memory write operation ordering >> > of overlapping pixels or samples. In contrast to >> > ARB_fragment_shader_interlock, INTEL_fragment_shader_ordering >> > instead of defining a critical region (which must be in main() and >> > under no flow control) provides a single function that acts like >> > a memory barrier that can be called under any control flow. >> > >> > Kevin Rogovin (2): >> > mesa: Add GL/GLSL plumbing for INTEL_fragment_shader_ordering. >> > i965: Add INTEL_fragment_shader_ordering support. >> > >> > docs/relnotes/18.3.0.html| 1 + >> > src/compiler/glsl/builtin_functions.cpp | 17 + >> > src/compiler/glsl/glsl_parser_extras.cpp | 1 + >> > src/compiler/glsl/glsl_parser_extras.h | 2 ++ >> > src/compiler/glsl/glsl_to_nir.cpp| 6 ++ >> > src/compiler/glsl/ir.h | 1 + >> > src/compiler/nir/nir_intrinsics.py | 1 + >> > src/intel/compiler/brw_fs_nir.cpp| 1 + >> > src/mesa/drivers/dri/i965/intel_extensions.c | 1 + >> > src/mesa/main/extensions_table.h | 1 + >> > src/mesa/main/mtypes.h | 1 + >> > 11 files changed, 33 insertions(+) >> > >> > -- >> > 2.17.1 >> > >> > ___ >> > mesa-dev mailing list >> > mesa-dev@lists.freedesktop.org >> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev >> ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering
Hi, On 28.08.2018 19:15, Rogovin, Kevin wrote: On the questions of tests: If people want, I can adapt the test in piglit for ARB_fragment_shader_interlock to this INTEL one. In general, I have an app/library that uses the extension and testing of that does definitely work on i965 (which should be utterly unsurprising since the produced assembly is identical). Did you mean this library: https://01.org/fast-ui-draw ? - Eero Indeed the current implementation in Mesa of ARB_fragment_shader_interlock is as flexible as the INTEL one since the compiler accepts code with the begin/end interlock anywhere since the only backend that supports it, i965, can easily accept it. I view the INTEL_fragment_shader_ordering implementation in a similar light as the NV_fragment_shader_interlock where it actually does not really do anything new, but signals to an application that the Mesa/i965 implementation allows more (and does it correctly) than the ARB/NV extensions restrict to. -Kevin ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering
Hi Mark, AFAIK there is no piglit test for this specific extension. However underneath the hood it reuses the functionality of ARB_fragment_shader_interlock, which has a test. I believe the only major difference between the two extensions is that unlike beginInvocationInterlockARB, beginFragmentShaderOrderingINTEL can be called from functions other than main(). If necessary it would be pretty straightforward to reuse most of the code for the ARB_fragment_shader_interlock test to create one for INTEL_fragment_shader_ordering. Thank you, Pam On Tue, Aug 28, 2018 at 6:41 PM Mark Janes wrote: > Is there a piglit test that verifies that this feature works properly? > > writes: > > > From: Kevin Rogovin > > > > INTEL_fragment_shader_ordering provides the ability for shaders > > to issue a call to gaurnantee memory write operation ordering > > of overlapping pixels or samples. In contrast to > > ARB_fragment_shader_interlock, INTEL_fragment_shader_ordering > > instead of defining a critical region (which must be in main() and > > under no flow control) provides a single function that acts like > > a memory barrier that can be called under any control flow. > > > > Kevin Rogovin (2): > > mesa: Add GL/GLSL plumbing for INTEL_fragment_shader_ordering. > > i965: Add INTEL_fragment_shader_ordering support. > > > > docs/relnotes/18.3.0.html| 1 + > > src/compiler/glsl/builtin_functions.cpp | 17 + > > src/compiler/glsl/glsl_parser_extras.cpp | 1 + > > src/compiler/glsl/glsl_parser_extras.h | 2 ++ > > src/compiler/glsl/glsl_to_nir.cpp| 6 ++ > > src/compiler/glsl/ir.h | 1 + > > src/compiler/nir/nir_intrinsics.py | 1 + > > src/intel/compiler/brw_fs_nir.cpp| 1 + > > src/mesa/drivers/dri/i965/intel_extensions.c | 1 + > > src/mesa/main/extensions_table.h | 1 + > > src/mesa/main/mtypes.h | 1 + > > 11 files changed, 33 insertions(+) > > > > -- > > 2.17.1 > > > > ___ > > mesa-dev mailing list > > mesa-dev@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering
Hi, Sorry for the doubly reply, but I wanted to add one more important bit in my thinking in addition to doing minimal code changes and following existing convention. The ARB/NV extensions defined a critical section where as the INTEL extension is just a barrier function. I suspect (but I do not know) that the NVIDIA hardware that supports the extension is implemented by some weird interfacing logic with the pixel backend and that implementation requires that there is just one critical section under no control flow whereas the INTEL is just a memory barrier. In that light, I think they should be 3 different intrinsics with the thinking that if the Nouveau driver adds support for the ARB/NV extension it will do the IR analysis to do what is needed for the critical-section style extension. -Kevin From: Rogovin, Kevin Sent: Tuesday, August 28, 2018 7:05 PM To: mesa-dev@lists.freedesktop.org Cc: Jerez Plata, Francisco Subject: Re: [Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering Hi, I followed the convention that was already present. The code from ARB/NV_fragment_shader_interlock had an intrinsic for begin critical section and an intrinsic for end critical section. I figured that since this extension has a completely different thinking (i.e. a memory barrier instead of a section) it warranted a new intrinsic. That and doing this way incurred the least amount of changes which I figured is always a good thing. -Kevin ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering
Hi, On the questions of tests: If people want, I can adapt the test in piglit for ARB_fragment_shader_interlock to this INTEL one. In general, I have an app/library that uses the extension and testing of that does definitely work on i965 (which should be utterly unsurprising since the produced assembly is identical). Indeed the current implementation in Mesa of ARB_fragment_shader_interlock is as flexible as the INTEL one since the compiler accepts code with the begin/end interlock anywhere since the only backend that supports it, i965, can easily accept it. I view the INTEL_fragment_shader_ordering implementation in a similar light as the NV_fragment_shader_interlock where it actually does not really do anything new, but signals to an application that the Mesa/i965 implementation allows more (and does it correctly) than the ARB/NV extensions restrict to. -Kevin ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering
Hi, I followed the convention that was already present. The code from ARB/NV_fragment_shader_interlock had an intrinsic for begin critical section and an intrinsic for end critical section. I figured that since this extension has a completely different thinking (i.e. a memory barrier instead of a section) it warranted a new intrinsic. That and doing this way incurred the least amount of changes which I figured is always a good thing. -Kevin ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering
Is there a piglit test that verifies that this feature works properly? writes: > From: Kevin Rogovin > > INTEL_fragment_shader_ordering provides the ability for shaders > to issue a call to gaurnantee memory write operation ordering > of overlapping pixels or samples. In contrast to > ARB_fragment_shader_interlock, INTEL_fragment_shader_ordering > instead of defining a critical region (which must be in main() and > under no flow control) provides a single function that acts like > a memory barrier that can be called under any control flow. > > Kevin Rogovin (2): > mesa: Add GL/GLSL plumbing for INTEL_fragment_shader_ordering. > i965: Add INTEL_fragment_shader_ordering support. > > docs/relnotes/18.3.0.html| 1 + > src/compiler/glsl/builtin_functions.cpp | 17 + > src/compiler/glsl/glsl_parser_extras.cpp | 1 + > src/compiler/glsl/glsl_parser_extras.h | 2 ++ > src/compiler/glsl/glsl_to_nir.cpp| 6 ++ > src/compiler/glsl/ir.h | 1 + > src/compiler/nir/nir_intrinsics.py | 1 + > src/intel/compiler/brw_fs_nir.cpp| 1 + > src/mesa/drivers/dri/i965/intel_extensions.c | 1 + > src/mesa/main/extensions_table.h | 1 + > src/mesa/main/mtypes.h | 1 + > 11 files changed, 33 insertions(+) > > -- > 2.17.1 > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering
Actually ignore my comment, I forgot that docs/features.txt is for non-vendor specific features only. I'll go ahead and push your patches. Sorry for the confusion. Thanks, Pam On Tue, Aug 28, 2018 at 1:04 PM Manolova, Plamena < plamena.manol...@intel.com> wrote: > Hi Kevin, > Could you also mark the extension as done in docs/features.txt? Otherwise > the series looks good to me > and with that tiny change it's Reviewed-by: Plamena Manolova < > plamena.manol...@intel.com> > > Thanks, > Pam > > On Mon, Aug 27, 2018 at 9:56 AM wrote: > >> From: Kevin Rogovin >> >> INTEL_fragment_shader_ordering provides the ability for shaders >> to issue a call to gaurnantee memory write operation ordering >> of overlapping pixels or samples. In contrast to >> ARB_fragment_shader_interlock, INTEL_fragment_shader_ordering >> instead of defining a critical region (which must be in main() and >> under no flow control) provides a single function that acts like >> a memory barrier that can be called under any control flow. >> >> Kevin Rogovin (2): >> mesa: Add GL/GLSL plumbing for INTEL_fragment_shader_ordering. >> i965: Add INTEL_fragment_shader_ordering support. >> >> docs/relnotes/18.3.0.html| 1 + >> src/compiler/glsl/builtin_functions.cpp | 17 + >> src/compiler/glsl/glsl_parser_extras.cpp | 1 + >> src/compiler/glsl/glsl_parser_extras.h | 2 ++ >> src/compiler/glsl/glsl_to_nir.cpp| 6 ++ >> src/compiler/glsl/ir.h | 1 + >> src/compiler/nir/nir_intrinsics.py | 1 + >> src/intel/compiler/brw_fs_nir.cpp| 1 + >> src/mesa/drivers/dri/i965/intel_extensions.c | 1 + >> src/mesa/main/extensions_table.h | 1 + >> src/mesa/main/mtypes.h | 1 + >> 11 files changed, 33 insertions(+) >> >> -- >> 2.17.1 >> >> ___ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev >> > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering
Hi Kevin, Could you also mark the extension as done in docs/features.txt? Otherwise the series looks good to me and with that tiny change it's Reviewed-by: Plamena Manolova < plamena.manol...@intel.com> Thanks, Pam On Mon, Aug 27, 2018 at 9:56 AM wrote: > From: Kevin Rogovin > > INTEL_fragment_shader_ordering provides the ability for shaders > to issue a call to gaurnantee memory write operation ordering > of overlapping pixels or samples. In contrast to > ARB_fragment_shader_interlock, INTEL_fragment_shader_ordering > instead of defining a critical region (which must be in main() and > under no flow control) provides a single function that acts like > a memory barrier that can be called under any control flow. > > Kevin Rogovin (2): > mesa: Add GL/GLSL plumbing for INTEL_fragment_shader_ordering. > i965: Add INTEL_fragment_shader_ordering support. > > docs/relnotes/18.3.0.html| 1 + > src/compiler/glsl/builtin_functions.cpp | 17 + > src/compiler/glsl/glsl_parser_extras.cpp | 1 + > src/compiler/glsl/glsl_parser_extras.h | 2 ++ > src/compiler/glsl/glsl_to_nir.cpp| 6 ++ > src/compiler/glsl/ir.h | 1 + > src/compiler/nir/nir_intrinsics.py | 1 + > src/intel/compiler/brw_fs_nir.cpp| 1 + > src/mesa/drivers/dri/i965/intel_extensions.c | 1 + > src/mesa/main/extensions_table.h | 1 + > src/mesa/main/mtypes.h | 1 + > 11 files changed, 33 insertions(+) > > -- > 2.17.1 > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering
From: Kevin Rogovin INTEL_fragment_shader_ordering provides the ability for shaders to issue a call to gaurnantee memory write operation ordering of overlapping pixels or samples. In contrast to ARB_fragment_shader_interlock, INTEL_fragment_shader_ordering instead of defining a critical region (which must be in main() and under no flow control) provides a single function that acts like a memory barrier that can be called under any control flow. Kevin Rogovin (2): mesa: Add GL/GLSL plumbing for INTEL_fragment_shader_ordering. i965: Add INTEL_fragment_shader_ordering support. docs/relnotes/18.3.0.html| 1 + src/compiler/glsl/builtin_functions.cpp | 17 + src/compiler/glsl/glsl_parser_extras.cpp | 1 + src/compiler/glsl/glsl_parser_extras.h | 2 ++ src/compiler/glsl/glsl_to_nir.cpp| 6 ++ src/compiler/glsl/ir.h | 1 + src/compiler/nir/nir_intrinsics.py | 1 + src/intel/compiler/brw_fs_nir.cpp| 1 + src/mesa/drivers/dri/i965/intel_extensions.c | 1 + src/mesa/main/extensions_table.h | 1 + src/mesa/main/mtypes.h | 1 + 11 files changed, 33 insertions(+) -- 2.17.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
