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

Jump to bottom

cmd/compile: improve implementation of OFORUNTIL inductive fact detection #40502

Closed
randall77 opened this issue Jul 30, 2020 · 4 comments
Closed

cmd/compile: improve implementation of OFORUNTIL inductive fact detection #40502

randall77 opened this issue Jul 30, 2020 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Performance
Milestone
Go1.16

Comments

@randall77
Copy link
Contributor

@randall77 randall77 commented Jul 30, 2020

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
@randall77 randall77 added this to the Go1.16 milestone Jul 30, 2020
@cagedmantis cagedmantis added the NeedsInvestigation label Jul 30, 2020
@choleraehyq
Copy link
Contributor

@choleraehyq choleraehyq commented Jul 31, 2020

I’ll do the isSameOrPlainSuccessor optimization and reenable these two test cases later.

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 31, 2020

Change https://golang.org/cl/246157 mentions this issue: cmd/compile: check indirect connection between if block and phi block in addLocalInductiveFacts

@randall77 randall77 mentioned this issue Aug 2, 2020
Closed
gopherbot pushed a commit that referenced this issue Nov 7, 2020
@choleraehyq @dmitshur
cmd/compile: check indirect connection between if block and phi block…
a6755fc 
… 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>
@zdjones
Copy link
Contributor

@zdjones zdjones commented Nov 25, 2020

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.

@randall77
Copy link
Contributor Author

@randall77 randall77 commented Dec 14, 2020

Ok, closing.

@golang golang locked and limited conversation to collaborators Dec 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Performance
Projects
None yet
Milestone
Go1.16
Development

No branches or pull requests
5 participants
@cagedmantis @zdjones @randall77 @gopherbot @choleraehyq