[Mesa-dev] [PATCH] nv50/ir: only stick one preret per function

[Ilia Mirkin]

On Sun, Oct 9, 2016 at 7:53 AM, Samuel Pitoiset
<samuel.pitoiset at gmail.com> wrote:
>
>
> On 10/09/2016 06:12 AM, Ilia Mirkin wrote:
>>
>> A function with multiple returns would have had multiple preret settings
>> at the top of the function. While this is unlikely to have caused issues
>> since we don't use funcitons in earnest, it could have in some cases
>
>
> s/funcitons/functions/ :)
>
> This looks good to me, but fyi this doesn't fix the regressions introduced
> with "nv50/ir: start LocalCSE with getFirst to merge PHI instructions".
> Something else is probably wrong.

I don't think I ever suggested it would. Just something I happened to
notice when looking at the compiled output of a shader. The only case
it could possibly fix is if there were >N preret's, which would
overflow the call stack and probably cause an error.

>
> Reviewed-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>

Thanks!

>
>
>> overflowed the call stack, in case a function had a lot of early
>> returns.
>>
>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>> ---
>>  src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp | 11
>> +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>> b/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>> index 5938226..6664366 100644
>> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>> @@ -3475,10 +3475,13 @@ Converter::handleInstruction(const struct
>> tgsi_full_instruction *insn)
>>        if (!isEndOfSubroutine(ip + 1)) {
>>           // insert a PRERET at the entry if this is an early return
>>           // (only needed for sharing code in the epilogue)
>> -         BasicBlock *pos = getBB();
>> -         setPosition(BasicBlock::get(func->cfg.getRoot()), false);
>> -         mkFlow(OP_PRERET, leave, CC_ALWAYS, NULL)->fixed = 1;
>> -         setPosition(pos, true);
>> +         BasicBlock *root = BasicBlock::get(func->cfg.getRoot());
>> +         if (root->getEntry() != NULL && root->getEntry()->op !=
>> OP_PRERET) {
>> +            BasicBlock *pos = getBB();
>> +            setPosition(root, false);
>> +            mkFlow(OP_PRERET, leave, CC_ALWAYS, NULL)->fixed = 1;
>> +            setPosition(pos, true);
>> +         }
>>        }
>>        mkFlow(OP_RET, NULL, CC_ALWAYS, NULL)->fixed = 1;
>>        bb->cfg.attach(&leave->cfg, Graph::Edge::CROSS);
>>
>