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 problem with union spliting during inlining, add regression test #42347

Merged
merged 4 commits into from
Sep 23, 2021

Conversation

ianatol
Copy link
Member

@ianatol ianatol commented Sep 22, 2021

Fix for gridap/Gridap.jl#657 and #42264

More detail from @Keno :
"The crash in #42264 showed up when attempting to record the new value for
an SSA rename during unionsplit inlining. Such a crash would only happen when
compact.idx == 1, which is an unusual condition, because it implies that no
statement of the original function had yet been processed (which is odd, because
the call being inlined must have been processed by this point). As it turns out, there
is currently exactly one case where this happens. If the inliner sees an _apply_iterate
(e.g. from splatting) of something that is not a Tuple or SimpleVector (thus
requiring calls to iterate to expand the value during apply), and if inference
was able to determine the total length of the resulting iteration, a special case early
inliner, will expand out the splat into explicit iterate calls. E.g. a call like:

 %r = tuple((x::Pair)...)

will be expanded out to

 %a = iterate(x::Pair)
 %b = getfield(%a, 1)
 %c = getfield(%a, 2)
 %d = iterate(x, %c)
 %e = getfield(%d, 1)
 %f = getfield(%d, 2)
 iterate(x, %f)
 %r = tuple(%b, %e)

where the inserted iterate calls may themselves be inlined. These newly inserted calls are
"pending nodes" during the actual inlining. Thus, if the original apply call was the first statement
of the function, these nodes would be processed before processing the statements in the function themselves.

In particular, this investigation also shows that compact.idx, which is the current location
in the function being inlined into is the wrong value to use for SSA renaming. Rather, we
need to use the SSA value of the just-inserted statement. In the absence of pending nodes,
these are equivalent concepts, but not for pending nodes. Fortunately the IncrementalCompact
iterator provides the old SSA value for just this reason and in fact, non-UnionSplit inlining was
already correct here. Thus, to fix the issue, simply adjust union splitting to work the same way
as a non-UnionSplit inline.

In coming up with the test case, an additional complication is that we currently do not perform this optimization for any calls where the apply call itself was unionsplit. It
is somewhat unusual (which explains why we haven't seen this much) for the apply
to not be union-split, but for the subsequent iterate to require union-splitting. In the
problem case, what happened is that for two out of the three union cases, the
iterate calls themselves would error, causing the resulting info object to look like
a non-unionsplit apply call, triggering this issue."

@KristofferC
Copy link
Member

Nice first contribution! ;)

@KristofferC KristofferC added backport 1.7 backport 1.6 Change should be backported to release-1.6 labels Sep 22, 2021
@JeffBezanson JeffBezanson added the compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) label Sep 22, 2021
@JeffBezanson JeffBezanson added the bugfix This change fixes an existing bug label Sep 22, 2021
Copy link
Member

@Keno Keno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For tricky compiler cases like these, particularly, when the actual diff is pretty small, I usually like to add a somewhat verbose commit message so that people who come across it later understand how the fix relates to the originally reported issue. Here, I would write something like:

The crash in #42264 showed up when attempting to record the new value for
an SSA rename during unionsplit inlining. Such a crash would only happen when
`compact.idx == 1`, which is an unusual condition, because it implies that no
statement of the original function had yet been processed (which is odd, because
the call being inlined must have been processed by this point). As it turns out, there
is currently exactly one case where this happens. If the inliner sees an `_apply_iterate`
(e.g. from splatting) of something that is not a Tuple or SimpleVector (thus
requiring calls to `iterate` to expand the value during `apply`), and if inference
was able to determine the total length of the resulting iteration, a special case early
inliner, will expand out the splat into explicit iterate calls. E.g. a call like:
    
     %r = tuple((x::Pair)...)

will be expanded out to

     %a = iterate(x::Pair)
     %b = getfield(%a, 1)
     %c = getfield(%a, 2)
     %d = iterate(x, %c)
     %e = getfield(%d, 1)
     %f = getfield(%d, 2)
     iterate(x, %f)
     %r = tuple(%b, %e)

where the inserted `iterate` calls may themselves be inlined. These newly inserted calls are
"pending nodes" during the actual inlining. Thus, if the original apply call was the first statement
of the function, these nodes would be processed before processing the statements in the function themselves.

In particular, this investigation also shows that `compact.idx`, which is the current location
in the function being inlined into is the wrong value to use for SSA renaming. Rather, we
need to use the SSA value of the just-inserted statement. In the absence of pending nodes,
these are equivalent concepts, but not for pending nodes. Fortunately the IncrementalCompact
iterator provides the old SSA value for just this reason and in fact, non-UnionSplit inlining was
already correct here. Thus, to fix the issue, simply adjust union splitting to work the same way
as a non-UnionSplit inline.

In coming up with the test case, an additional complication is that we currently do not perform this optimization for any calls where the apply call itself was unionsplit. It
is somewhat unusual (which explains why we haven't seen this much) for the apply
to not be union-split, but for the subsequent `iterate` to require union-splitting. In the
problem case, what happened is that for two out of the three union cases, the
`iterate` calls themselves would error, causing the resulting info object to look like
a non-unionsplit apply call, triggering this issue.

@dkarrasch dkarrasch merged commit b5f3a99 into JuliaLang:master Sep 23, 2021
KristofferC pushed a commit that referenced this pull request Sep 23, 2021
…42347)

The crash in #42264 showed up when attempting to record the new value for
an SSA rename during unionsplit inlining. Such a crash would only happen when
`compact.idx == 1`, which is an unusual condition, because it implies that no
statement of the original function had yet been processed (which is odd, because
the call being inlined must have been processed by this point). As it turns out, there
is currently exactly one case where this happens. If the inliner sees an `_apply_iterate`
(e.g. from splatting) of something that is not a Tuple or SimpleVector (thus
requiring calls to `iterate` to expand the value during `apply`), and if inference
was able to determine the total length of the resulting iteration, a special case early
inliner, will expand out the splat into explicit iterate calls. E.g. a call like:

     %r = tuple((x::Pair)...)

will be expanded out to

     %a = iterate(x::Pair)
     %b = getfield(%a, 1)
     %c = getfield(%a, 2)
     %d = iterate(x, %c)
     %e = getfield(%d, 1)
     %f = getfield(%d, 2)
     iterate(x, %f)
     %r = tuple(%b, %e)

where the inserted `iterate` calls may themselves be inlined. These newly inserted calls are
"pending nodes" during the actual inlining. Thus, if the original apply call was the first statement
of the function, these nodes would be processed before processing the statements in the function themselves.

In particular, this investigation also shows that `compact.idx`, which is the current location
in the function being inlined into is the wrong value to use for SSA renaming. Rather, we
need to use the SSA value of the just-inserted statement. In the absence of pending nodes,
these are equivalent concepts, but not for pending nodes. Fortunately the IncrementalCompact
iterator provides the old SSA value for just this reason and in fact, non-UnionSplit inlining was
already correct here. Thus, to fix the issue, simply adjust union splitting to work the same way
as a non-UnionSplit inline.

In coming up with the test case, an additional complication is that we currently do not perform this optimization for any calls where the apply call itself was unionsplit. It
is somewhat unusual (which explains why we haven't seen this much) for the apply
to not be union-split, but for the subsequent `iterate` to require union-splitting. In the
problem case, what happened is that for two out of the three union cases, the
`iterate` calls themselves would error, causing the resulting info object to look like
a non-unionsplit apply call, triggering this issue.

Co-authored-by: Keno Fischer <[email protected]>
(cherry picked from commit b5f3a99)
KristofferC pushed a commit that referenced this pull request Sep 28, 2021
…42347)

The crash in #42264 showed up when attempting to record the new value for
an SSA rename during unionsplit inlining. Such a crash would only happen when
`compact.idx == 1`, which is an unusual condition, because it implies that no
statement of the original function had yet been processed (which is odd, because
the call being inlined must have been processed by this point). As it turns out, there
is currently exactly one case where this happens. If the inliner sees an `_apply_iterate`
(e.g. from splatting) of something that is not a Tuple or SimpleVector (thus
requiring calls to `iterate` to expand the value during `apply`), and if inference
was able to determine the total length of the resulting iteration, a special case early
inliner, will expand out the splat into explicit iterate calls. E.g. a call like:
    
     %r = tuple((x::Pair)...)

will be expanded out to

     %a = iterate(x::Pair)
     %b = getfield(%a, 1)
     %c = getfield(%a, 2)
     %d = iterate(x, %c)
     %e = getfield(%d, 1)
     %f = getfield(%d, 2)
     iterate(x, %f)
     %r = tuple(%b, %e)

where the inserted `iterate` calls may themselves be inlined. These newly inserted calls are
"pending nodes" during the actual inlining. Thus, if the original apply call was the first statement
of the function, these nodes would be processed before processing the statements in the function themselves.

In particular, this investigation also shows that `compact.idx`, which is the current location
in the function being inlined into is the wrong value to use for SSA renaming. Rather, we
need to use the SSA value of the just-inserted statement. In the absence of pending nodes,
these are equivalent concepts, but not for pending nodes. Fortunately the IncrementalCompact
iterator provides the old SSA value for just this reason and in fact, non-UnionSplit inlining was
already correct here. Thus, to fix the issue, simply adjust union splitting to work the same way
as a non-UnionSplit inline.

In coming up with the test case, an additional complication is that we currently do not perform this optimization for any calls where the apply call itself was unionsplit. It
is somewhat unusual (which explains why we haven't seen this much) for the apply
to not be union-split, but for the subsequent `iterate` to require union-splitting. In the
problem case, what happened is that for two out of the three union cases, the
`iterate` calls themselves would error, causing the resulting info object to look like
a non-unionsplit apply call, triggering this issue.

Co-authored-by: Keno Fischer <[email protected]>
KristofferC pushed a commit that referenced this pull request Oct 29, 2021
…42347)

The crash in #42264 showed up when attempting to record the new value for
an SSA rename during unionsplit inlining. Such a crash would only happen when
`compact.idx == 1`, which is an unusual condition, because it implies that no
statement of the original function had yet been processed (which is odd, because
the call being inlined must have been processed by this point). As it turns out, there
is currently exactly one case where this happens. If the inliner sees an `_apply_iterate`
(e.g. from splatting) of something that is not a Tuple or SimpleVector (thus
requiring calls to `iterate` to expand the value during `apply`), and if inference
was able to determine the total length of the resulting iteration, a special case early
inliner, will expand out the splat into explicit iterate calls. E.g. a call like:

     %r = tuple((x::Pair)...)

will be expanded out to

     %a = iterate(x::Pair)
     %b = getfield(%a, 1)
     %c = getfield(%a, 2)
     %d = iterate(x, %c)
     %e = getfield(%d, 1)
     %f = getfield(%d, 2)
     iterate(x, %f)
     %r = tuple(%b, %e)

where the inserted `iterate` calls may themselves be inlined. These newly inserted calls are
"pending nodes" during the actual inlining. Thus, if the original apply call was the first statement
of the function, these nodes would be processed before processing the statements in the function themselves.

In particular, this investigation also shows that `compact.idx`, which is the current location
in the function being inlined into is the wrong value to use for SSA renaming. Rather, we
need to use the SSA value of the just-inserted statement. In the absence of pending nodes,
these are equivalent concepts, but not for pending nodes. Fortunately the IncrementalCompact
iterator provides the old SSA value for just this reason and in fact, non-UnionSplit inlining was
already correct here. Thus, to fix the issue, simply adjust union splitting to work the same way
as a non-UnionSplit inline.

In coming up with the test case, an additional complication is that we currently do not perform this optimization for any calls where the apply call itself was unionsplit. It
is somewhat unusual (which explains why we haven't seen this much) for the apply
to not be union-split, but for the subsequent `iterate` to require union-splitting. In the
problem case, what happened is that for two out of the three union cases, the
`iterate` calls themselves would error, causing the resulting info object to look like
a non-unionsplit apply call, triggering this issue.

Co-authored-by: Keno Fischer <[email protected]>
(cherry picked from commit b5f3a99)
KristofferC pushed a commit that referenced this pull request Oct 29, 2021
…42347)

The crash in #42264 showed up when attempting to record the new value for
an SSA rename during unionsplit inlining. Such a crash would only happen when
`compact.idx == 1`, which is an unusual condition, because it implies that no
statement of the original function had yet been processed (which is odd, because
the call being inlined must have been processed by this point). As it turns out, there
is currently exactly one case where this happens. If the inliner sees an `_apply_iterate`
(e.g. from splatting) of something that is not a Tuple or SimpleVector (thus
requiring calls to `iterate` to expand the value during `apply`), and if inference
was able to determine the total length of the resulting iteration, a special case early
inliner, will expand out the splat into explicit iterate calls. E.g. a call like:

     %r = tuple((x::Pair)...)

will be expanded out to

     %a = iterate(x::Pair)
     %b = getfield(%a, 1)
     %c = getfield(%a, 2)
     %d = iterate(x, %c)
     %e = getfield(%d, 1)
     %f = getfield(%d, 2)
     iterate(x, %f)
     %r = tuple(%b, %e)

where the inserted `iterate` calls may themselves be inlined. These newly inserted calls are
"pending nodes" during the actual inlining. Thus, if the original apply call was the first statement
of the function, these nodes would be processed before processing the statements in the function themselves.

In particular, this investigation also shows that `compact.idx`, which is the current location
in the function being inlined into is the wrong value to use for SSA renaming. Rather, we
need to use the SSA value of the just-inserted statement. In the absence of pending nodes,
these are equivalent concepts, but not for pending nodes. Fortunately the IncrementalCompact
iterator provides the old SSA value for just this reason and in fact, non-UnionSplit inlining was
already correct here. Thus, to fix the issue, simply adjust union splitting to work the same way
as a non-UnionSplit inline.

In coming up with the test case, an additional complication is that we currently do not perform this optimization for any calls where the apply call itself was unionsplit. It
is somewhat unusual (which explains why we haven't seen this much) for the apply
to not be union-split, but for the subsequent `iterate` to require union-splitting. In the
problem case, what happened is that for two out of the three union cases, the
`iterate` calls themselves would error, causing the resulting info object to look like
a non-unionsplit apply call, triggering this issue.

Co-authored-by: Keno Fischer <[email protected]>
(cherry picked from commit b5f3a99)
KristofferC pushed a commit that referenced this pull request Nov 11, 2021
…42347)

The crash in #42264 showed up when attempting to record the new value for
an SSA rename during unionsplit inlining. Such a crash would only happen when
`compact.idx == 1`, which is an unusual condition, because it implies that no
statement of the original function had yet been processed (which is odd, because
the call being inlined must have been processed by this point). As it turns out, there
is currently exactly one case where this happens. If the inliner sees an `_apply_iterate`
(e.g. from splatting) of something that is not a Tuple or SimpleVector (thus
requiring calls to `iterate` to expand the value during `apply`), and if inference
was able to determine the total length of the resulting iteration, a special case early
inliner, will expand out the splat into explicit iterate calls. E.g. a call like:

     %r = tuple((x::Pair)...)

will be expanded out to

     %a = iterate(x::Pair)
     %b = getfield(%a, 1)
     %c = getfield(%a, 2)
     %d = iterate(x, %c)
     %e = getfield(%d, 1)
     %f = getfield(%d, 2)
     iterate(x, %f)
     %r = tuple(%b, %e)

where the inserted `iterate` calls may themselves be inlined. These newly inserted calls are
"pending nodes" during the actual inlining. Thus, if the original apply call was the first statement
of the function, these nodes would be processed before processing the statements in the function themselves.

In particular, this investigation also shows that `compact.idx`, which is the current location
in the function being inlined into is the wrong value to use for SSA renaming. Rather, we
need to use the SSA value of the just-inserted statement. In the absence of pending nodes,
these are equivalent concepts, but not for pending nodes. Fortunately the IncrementalCompact
iterator provides the old SSA value for just this reason and in fact, non-UnionSplit inlining was
already correct here. Thus, to fix the issue, simply adjust union splitting to work the same way
as a non-UnionSplit inline.

In coming up with the test case, an additional complication is that we currently do not perform this optimization for any calls where the apply call itself was unionsplit. It
is somewhat unusual (which explains why we haven't seen this much) for the apply
to not be union-split, but for the subsequent `iterate` to require union-splitting. In the
problem case, what happened is that for two out of the three union cases, the
`iterate` calls themselves would error, causing the resulting info object to look like
a non-unionsplit apply call, triggering this issue.

Co-authored-by: Keno Fischer <[email protected]>
(cherry picked from commit b5f3a99)
@KristofferC KristofferC removed the backport 1.6 Change should be backported to release-1.6 label Nov 13, 2021
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
…uliaLang#42347)

The crash in JuliaLang#42264 showed up when attempting to record the new value for
an SSA rename during unionsplit inlining. Such a crash would only happen when
`compact.idx == 1`, which is an unusual condition, because it implies that no
statement of the original function had yet been processed (which is odd, because
the call being inlined must have been processed by this point). As it turns out, there
is currently exactly one case where this happens. If the inliner sees an `_apply_iterate`
(e.g. from splatting) of something that is not a Tuple or SimpleVector (thus
requiring calls to `iterate` to expand the value during `apply`), and if inference
was able to determine the total length of the resulting iteration, a special case early
inliner, will expand out the splat into explicit iterate calls. E.g. a call like:
    
     %r = tuple((x::Pair)...)

will be expanded out to

     %a = iterate(x::Pair)
     %b = getfield(%a, 1)
     %c = getfield(%a, 2)
     %d = iterate(x, %c)
     %e = getfield(%d, 1)
     %f = getfield(%d, 2)
     iterate(x, %f)
     %r = tuple(%b, %e)

where the inserted `iterate` calls may themselves be inlined. These newly inserted calls are
"pending nodes" during the actual inlining. Thus, if the original apply call was the first statement
of the function, these nodes would be processed before processing the statements in the function themselves.

In particular, this investigation also shows that `compact.idx`, which is the current location
in the function being inlined into is the wrong value to use for SSA renaming. Rather, we
need to use the SSA value of the just-inserted statement. In the absence of pending nodes,
these are equivalent concepts, but not for pending nodes. Fortunately the IncrementalCompact
iterator provides the old SSA value for just this reason and in fact, non-UnionSplit inlining was
already correct here. Thus, to fix the issue, simply adjust union splitting to work the same way
as a non-UnionSplit inline.

In coming up with the test case, an additional complication is that we currently do not perform this optimization for any calls where the apply call itself was unionsplit. It
is somewhat unusual (which explains why we haven't seen this much) for the apply
to not be union-split, but for the subsequent `iterate` to require union-splitting. In the
problem case, what happened is that for two out of the three union cases, the
`iterate` calls themselves would error, causing the resulting info object to look like
a non-unionsplit apply call, triggering this issue.

Co-authored-by: Keno Fischer <[email protected]>
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
…uliaLang#42347)

The crash in JuliaLang#42264 showed up when attempting to record the new value for
an SSA rename during unionsplit inlining. Such a crash would only happen when
`compact.idx == 1`, which is an unusual condition, because it implies that no
statement of the original function had yet been processed (which is odd, because
the call being inlined must have been processed by this point). As it turns out, there
is currently exactly one case where this happens. If the inliner sees an `_apply_iterate`
(e.g. from splatting) of something that is not a Tuple or SimpleVector (thus
requiring calls to `iterate` to expand the value during `apply`), and if inference
was able to determine the total length of the resulting iteration, a special case early
inliner, will expand out the splat into explicit iterate calls. E.g. a call like:
    
     %r = tuple((x::Pair)...)

will be expanded out to

     %a = iterate(x::Pair)
     %b = getfield(%a, 1)
     %c = getfield(%a, 2)
     %d = iterate(x, %c)
     %e = getfield(%d, 1)
     %f = getfield(%d, 2)
     iterate(x, %f)
     %r = tuple(%b, %e)

where the inserted `iterate` calls may themselves be inlined. These newly inserted calls are
"pending nodes" during the actual inlining. Thus, if the original apply call was the first statement
of the function, these nodes would be processed before processing the statements in the function themselves.

In particular, this investigation also shows that `compact.idx`, which is the current location
in the function being inlined into is the wrong value to use for SSA renaming. Rather, we
need to use the SSA value of the just-inserted statement. In the absence of pending nodes,
these are equivalent concepts, but not for pending nodes. Fortunately the IncrementalCompact
iterator provides the old SSA value for just this reason and in fact, non-UnionSplit inlining was
already correct here. Thus, to fix the issue, simply adjust union splitting to work the same way
as a non-UnionSplit inline.

In coming up with the test case, an additional complication is that we currently do not perform this optimization for any calls where the apply call itself was unionsplit. It
is somewhat unusual (which explains why we haven't seen this much) for the apply
to not be union-split, but for the subsequent `iterate` to require union-splitting. In the
problem case, what happened is that for two out of the three union cases, the
`iterate` calls themselves would error, causing the resulting info object to look like
a non-unionsplit apply call, triggering this issue.

Co-authored-by: Keno Fischer <[email protected]>
staticfloat pushed a commit that referenced this pull request Dec 23, 2022
…42347)

The crash in #42264 showed up when attempting to record the new value for
an SSA rename during unionsplit inlining. Such a crash would only happen when
`compact.idx == 1`, which is an unusual condition, because it implies that no
statement of the original function had yet been processed (which is odd, because
the call being inlined must have been processed by this point). As it turns out, there
is currently exactly one case where this happens. If the inliner sees an `_apply_iterate`
(e.g. from splatting) of something that is not a Tuple or SimpleVector (thus
requiring calls to `iterate` to expand the value during `apply`), and if inference
was able to determine the total length of the resulting iteration, a special case early
inliner, will expand out the splat into explicit iterate calls. E.g. a call like:

     %r = tuple((x::Pair)...)

will be expanded out to

     %a = iterate(x::Pair)
     %b = getfield(%a, 1)
     %c = getfield(%a, 2)
     %d = iterate(x, %c)
     %e = getfield(%d, 1)
     %f = getfield(%d, 2)
     iterate(x, %f)
     %r = tuple(%b, %e)

where the inserted `iterate` calls may themselves be inlined. These newly inserted calls are
"pending nodes" during the actual inlining. Thus, if the original apply call was the first statement
of the function, these nodes would be processed before processing the statements in the function themselves.

In particular, this investigation also shows that `compact.idx`, which is the current location
in the function being inlined into is the wrong value to use for SSA renaming. Rather, we
need to use the SSA value of the just-inserted statement. In the absence of pending nodes,
these are equivalent concepts, but not for pending nodes. Fortunately the IncrementalCompact
iterator provides the old SSA value for just this reason and in fact, non-UnionSplit inlining was
already correct here. Thus, to fix the issue, simply adjust union splitting to work the same way
as a non-UnionSplit inline.

In coming up with the test case, an additional complication is that we currently do not perform this optimization for any calls where the apply call itself was unionsplit. It
is somewhat unusual (which explains why we haven't seen this much) for the apply
to not be union-split, but for the subsequent `iterate` to require union-splitting. In the
problem case, what happened is that for two out of the three union cases, the
`iterate` calls themselves would error, causing the resulting info object to look like
a non-unionsplit apply call, triggering this issue.

Co-authored-by: Keno Fischer <[email protected]>
(cherry picked from commit b5f3a99)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug compiler:optimizer Optimization passes (mostly in base/compiler/ssair/)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants