Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix return statements ignoring assignments in favor of an unreachable one #1051

Merged
merged 1 commit into from Aug 15, 2019

Conversation

@marsupial
Copy link
Contributor

commented Aug 14, 2019

Description

Assignments that occur in a block whichreturn will be ignored.
This may only be triggered when a block's condition is const-folded and the parent block contains other assignments.

Tests

Added testsuite/bug-return
Running shading tests now.

@lgritz

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Did you maybe not git add a few of the files?

@marsupial

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

What files ?

@lgritz

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Well, I don't know what's missing if it's missing.

For starters, look at the list of tests you added to CMakeLists.txt and see if all the new tests have properly committed files.

@marsupial

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

I only added one test, just Xcode shuffled the columns.
The test I added and others pass.
Windows issue seems to be unrelated.

@lgritz

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

ah, ok, maybe my bad then

@lgritz

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

OK, I think the if statements in your test are not really relevant here. The basic situation is that

a = 10;
... stuff not involving `a`...
a = 20;

sees two assignments within the same basic block, and because a is not used between the two assignments, the first one can be eliminated.

But this optimization ("stale assignment elision") was wrong for this modified code:

a = 10;
return;
a = 20;

because the return changes things -- the second assignment won't be executed, and therefore should not retroactively eliminate the earlier assignment. The correct final value of a is indeed 10.

I see that your code fixes this particular case, because once it sees the return (which underneath is the 'exit' instruction, if it's returning from the main shader rather than just a function), it turns subsequent ops within the same basic block into nops:

a = 10;
exit;
nop   # was a=20 originally

But...

I'm struggling to convince myself that your solution is (and always will be) correct. There is an upstream logic error, which is that An exit instruction should terminate a basic block but was not recognized as such.

The real bug is in OSOProcessorBase::find_basic_blocks:

    // 'break', 'continue', and 'return' also cause the next
    // statement to begin a new basic block.
    if (op.opname() == u_break || op.opname() == u_continue ||
            op.opname() == u_return)
        block_begin[opnum+1] = true;

This should also be testing for the exit instruction.

I'm worried that there are other subtle bugs that could be latent, or be introduced in the future, from getting this wrong.

@@ -1084,7 +1085,7 @@ OSOProcessorBase::find_basic_blocks ()
// 'break', 'continue', and 'return' also cause the next
// statement to begin a new basic block.
if (op.opname() == u_break || op.opname() == u_continue ||
op.opname() == u_return)
op.opname() == u_return || op.opname() == u_exit)

This comment has been minimized.

Copy link
@lgritz

lgritz Aug 15, 2019

Member

Let's update the comment above as well.

@@ -2121,6 +2122,26 @@ RuntimeOptimizer::optimize_ops (int beginop, int endop,
// Nothing below here to do for no-ops, take early out.
if (op->opname() == u_nop)
continue;
// If an exit or return was reached, kill the remaining instructions
if (optimize() && (op->opname() == u_return || op->opname() == u_exit)) {

This comment has been minimized.

Copy link
@lgritz

lgritz Aug 15, 2019

Member

I'm not sure this is correct. For exit, yes, but for return, it would be wrong to eliminate all the rest of the ops in the shader. You would want to clear at most the rest of the ops in the function, right?

This comment has been minimized.

Copy link
@marsupial

marsupial Aug 15, 2019

Author Contributor

Yes, perhaps the comment is wrong.
optimize_ops will be called recursively for functions, with endop pointing to the function end.

turn_into_nop (*op, "eliminated unreachable op");
}
} else {
// Nop the remaining instructions, preserving the end-op

This comment has been minimized.

Copy link
@lgritz

lgritz Aug 15, 2019

Member

Let's update this comment to be a little more explanatory:

// Since we hit an 'exit' in unconditionally-executed code, it is safe to
// eliminate all the remaining code up to final exit op.

I think this part is correct, if you fix it so that this is happening only for exit and not for return.

ASSERT(skipops == 0 && "Unexpected skipops");
if (m_in_conditional[opnum]) {
// Nop the remaining instructions in the same basic block
while (next_block_instruction(opnum)) {

This comment has been minimized.

Copy link
@lgritz

lgritz Aug 15, 2019

Member

Now that you fixed it so that it recognizes exit as the end of a basic block, does this code actually do anything? Seems like the while loop will never execute its body since the exit instruction is now -- correctly -- the last instruction in its basic block.

This comment has been minimized.

Copy link
@marsupial

marsupial Aug 15, 2019

Author Contributor

Yes, I think that should be while (m_in_conditional[++opnum])

@marsupial

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

Is it worth splitting out the no-op optimizations into a separate PR and just get the exit fix in ?

@lgritz

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

Yeah, probably. Let's have one PR that is just the important bug fix to OSOProcessorBase::find_basic_blocks, and I will backport that to the release branch. Then we can have a second PR for the new optimization, and for safety we'll put that in master only.

rzulak

@marsupial marsupial force-pushed the marsupial:PR/bug-return branch from 85ea39f to 0315360 Aug 15, 2019

@lgritz
lgritz approved these changes Aug 15, 2019
Copy link
Member

left a comment

LGTM

@lgritz lgritz merged commit 5ee8f86 into imageworks:master Aug 15, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgritz added a commit to lgritz/OpenShadingLanguage that referenced this pull request Aug 16, 2019
Fix `exit` not being recognized as basic block end (imageworks#1051)
Stale assignment elision was broken in the presence of `exit`.

In general

    a = 10;
    ... stuff not involving `a`...
    a = 20;

sees two assignments within the same basic block, and because `a` is not used between the two assignments, the first one can be eliminated.

But this optimization ("stale assignment elision") was wrong for this modified code:

    a = 10;
    return;  // note: `return` in the main shader body is the same as `exit()`
    a = 20;

because the `return` changes things -- the second assignment won't be executed, and therefore should not retroactively eliminate the earlier assignment. The correct final value of a is indeed 10.

The problem boiled down to a bug in OSOProcessorBase::find_basic_blocks, where the `exit` op was simply not recognized as terminating its basic block (since it is a jump).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.