Enable visibility expression builder usage while filtering BFS on def-use DAG#4506
Draft
copybara-service[bot] wants to merge 1 commit into
Draft
Enable visibility expression builder usage while filtering BFS on def-use DAG#4506copybara-service[bot] wants to merge 1 commit into
copybara-service[bot] wants to merge 1 commit into
Conversation
…-use DAG This change ensures `BuildVisibilityIRExpr` always returns a valid visibility expression regardless of `conditional_edges` provided. If none are provided, then the function would return always_visible. If every edge in the def-use DAG from `node` down to terminal nodes is provided, then the expression would be exact, assuming no BDD overflow, and conservative otherwise. Providing only a subset of the entire DAG would produce a smaller and conservative expression, perhaps less conservative if that subset is chosen wisely. BuildVisibilityIRExpr had a bug where the short-circuit path when you provided a single-element conditional_edges set assumed that that single edge was a sufficient visibility expression to account for every path through the DAG. This assumption was made because resource sharing was using BuildVisibilityIRExpr in this way if it could prove mutual exclusivity via SingleSelectVisibilityAnalysis. For more context: * BuildVisibilityIRExprFromEdges computes visibility as OR(vis_node_through_user_i AND vis_user_i) where if the edge (node->user) is NOT in conditional_edges we assume vis_node_through_user_i is always true. This function is conservative. Using conditional_edgescan help prevent BDD overflow. * The test helper BuildDefaultVisibilityExpr uses GetEdgesForMutuallyExclusiveVisibilityExpr to determine a subset of conditional_edges that helps BuildVisibilityIRExprFromEdgesnot overflow. PiperOrigin-RevId: 941218304
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Enable visibility expression builder usage while filtering BFS on def-use DAG
This change ensures
BuildVisibilityIRExpralways returns a valid visibility expression regardless ofconditional_edgesprovided. If none are provided, then the function would return always_visible. If every edge in the def-use DAG fromnodedown to terminal nodes is provided, then the expression would be exact, assuming no BDD overflow, and conservative otherwise. Providing only a subset of the entire DAG would produce a smaller and conservative expression, perhaps less conservative if that subset is chosen wisely.BuildVisibilityIRExpr had a bug where the short-circuit path when you provided a single-element conditional_edges set assumed that that single edge was a sufficient visibility expression to account for every path through the DAG. This assumption was made because resource sharing was using BuildVisibilityIRExpr in this way if it could prove mutual exclusivity via SingleSelectVisibilityAnalysis.
For more context: