[Mesa-dev] [PATCH 10/14] glsl_to_tgsi: remove subroutine support

[Marek Olšák]

I'm adding back this:

       options->EmitNoMainReturn =
          !screen->get_shader_param(screen, sh, PIPE_SHADER_CAP_SUBROUTINES);

And:

diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
index 293654c..5f28d07 100644
--- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
+++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
@@ -4176,7 +4176,9 @@ glsl_to_tgsi_visitor::visit(ir_texture *ir)
 void
 glsl_to_tgsi_visitor::visit(ir_return *ir)
 {
-   assert(0);
+   assert(!ir->get_value());
+
+   emit_asm(ir, TGSI_OPCODE_RET);
 }

 void
@@ -4416,7 +4418,8 @@ glsl_to_tgsi_visitor::simplify_cmp(void)
           inst->dst[1].reladdr || inst->dst[1].reladdr2 ||
           tgsi_get_opcode_info(inst->op)->is_branch ||
           inst->op == TGSI_OPCODE_CONT ||
-          inst->op == TGSI_OPCODE_END) {
+          inst->op == TGSI_OPCODE_END ||
+          inst->op == TGSI_OPCODE_RET) {
          break;
       }


Hopefully that should restore the RET support.

Marek


On Mon, Oct 17, 2016 at 4:12 PM, Marek Olšák <maraeo at gmail.com> wrote:
> On Mon, Oct 17, 2016 at 4:10 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>> On Mon, Oct 17, 2016 at 9:59 AM, Marek Olšák <maraeo at gmail.com> wrote:
>>> On Mon, Oct 17, 2016 at 3:48 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>>> On Mon, Oct 17, 2016 at 9:46 AM, Marek Olšák <maraeo at gmail.com> wrote:
>>>>> On Mon, Oct 17, 2016 at 3:44 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>>>>> nouveau supports PIPE_SHADER_CAP_SUBROUTINES and properly details with
>>>>>> RET opcodes. The alternative is that the st lowers the whole thing
>>>>>> into a loop which adds IMHO unnecessary complexity to the resulting
>>>>>> code. Any reason not to leave that in place?
>>>>>
>>>>> It had already been disabled and probably broken too. If you have no
>>>>> interest in fixing and using it within a reasonable time frame, there
>>>>> is no reason for the code to be there.
>>>>
>>>> What's broken about it? To the best of my knowledge, it's working
>>>> fine. I'm specifically talking about RET (from the "MAIN" function),
>>>> not subroutines in general.
>>>
>>> OK. I thought you were talking about subroutines.
>>>
>>> The RET opcode support can stay there if somebody wants it very much.
>>>
>>> However, have you seen any apps using "return" in "main"?
>>
>> Yes. Among other things, a lot of compute shaders do if (foo) return;
>> somewhere at the top. See this example from F1 2015:
>> http://hastebin.com/vacevigoyi.go - I've seen it in the middle too
>> though.
>>
>>> Is there any serious performance concern with lowering "return" to a
>>> conditional branch, such that it's beneficial for drivers to implement
>>> RET?
>>
>> Depends on the driver. Nouveau's control flow analysis isn't extremely
>> sophisticated to undo the lowering that the GLSL compiler does. Which
>> means extra instructions, and extra RA complexity since the whole main
>> function ends up inside a loop.
>>
>>> If so, shouldn't it be fixed in the GLSL compiler instead?
>>
>> Well, from what I recall, the GLSL compiler's lowering goes something like
>>
>> void main() {
>>   if (foo) return;
>>   ...
>> }
>>
>> to
>>
>> void main() {
>>   while (true) {
>>      if (foo)
>>         break;
>>      ...
>>      break;
>>   }
>> }
>>
>> With additional complexity if the return is inside a for/while loop itself.
>>
>>> Is it worth having a rarely-used codepath in glsl_to_tgsi that most
>>> drivers skip anyway?
>>
>> It's 2 lines of code... one to stop the GLSL compiler from doing the
>> lowering, and another to emit the RET opcode when you see the
>> ir_op_return or whatever it's called. IMHO not such a great burden.
>>
>> All the other subroutines stuff can go as far as I'm concerned, I
>> doubt anyone will care to implement glsl subroutines that way.
>>
>>   -ilia
>
> OK, sounds good.
>
> Marek