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
cmd/compile: improve implementation of OFORUNTIL inductive fact detection #40502
Comments
I’ll do the isSameOrPlainSuccessor optimization and reenable these two test cases later. |
Change https://golang.org/cl/246157 mentions this issue: |
… in addLocalInductiveFacts CL 244579 added guard clauses to prevent a faulty state that was possible under the incorrect logic of the uniquePred loop in addLocalInductiveFacts. That faulty state was still making the intended optimization, but not for the correct reason. Removing the faulty state also removed the overly permissive application of the optimization, and therefore made these two tests fail. We disabled the tests of this optimization in CL 244579 to allow us to quickly apply the fix in the CL. This CL now corrects the logic of the uniquePred loop in order to apply the optimization correctly. The comment above the uniquePred loop says that it will follow unique predecessors until it reaches a join point. Without updating the child node on each iteration, it cannot follow the chain of unique predecessors more than one step. Adding the update to the child node on each iteration of the loop allows the logic to follow the chain of unique predecessors until reaching a join point (because a non-unique predecessor will signify a join point). Updates #40502. Change-Id: I23d8367046a2ab3ce4be969631f9ba15dc533e6c Reviewed-on: https://go-review.googlesource.com/c/go/+/246157 Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: David Chase <drchase@google.com> Trust: Dmitri Shuralyov <dmitshur@golang.org>
I see that we listed https://golang.org/cl/246157 as an update to this issue, not a fix. I think that should have been a fix. This issue was just to correct/reenable the OFORUNTIL fact detection that was made more conservative by the fix in #40367. 246157 corrected that logic. |
Ok, closing. |
This issue is for followon improvements to #40367. We had to disable some optimizations for that issue, which we should reenable/improve for 1.16.
@choleraehyq @zdjones @dr2chase @cherrymui
The text was updated successfully, but these errors were encountered: