[AutoDiff] Revamp usefulness propagation in activity analysis. #28225
Conversation
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
|
Created PR as a draft because I feel there's room for improvement. |
dan-zheng
commented
Nov 13, 2019
| @@ -1444,8 +1444,18 @@ class DifferentiableActivityInfo { | |||
| void setUseful(SILValue value, unsigned dependentVariableIndex); | |||
There was a problem hiding this comment.
Note: currently, setUseful has two users (setUsefulAndPropagateToOperands and propagateUsefulThroughAddress), so it hasn't been inlined.
| auto *inst = value->getDefiningInstruction(); | ||
| if (!inst) | ||
| return; | ||
| propagateUseful(inst, dependentVariableIndex); |
There was a problem hiding this comment.
Note: propagateUseful cannot be inlined because it has multiple users (called multiple times in propagateUsefulThroughAddress.
| propagateUsefulThroughAddress(value, dependentVariableIndex); | ||
| return; | ||
| } | ||
| setUseful(value, dependentVariableIndex); |
There was a problem hiding this comment.
Note: both setUsefulAndPropagateToOperands and propagateUsefulThroughAddress call isUseful and setUseful, which seems suboptimal. I tried removing setUseful from one of them but ran into infinite loops.
Related: I think setUsefulAndPropagateToOperands should be the primary entry point for propagating usefulness, so I eliminated most direct calls to propagateUsefulThroughAddress.
rxwei
reviewed
Nov 13, 2019
dan-zheng
force-pushed
the
autodiff-fix-activity-analysis
branch
2 times, most recently
from
98cb4ec to
05e0b86
Compare
Useful values are those that contribute to (specific) dependent variables,
i.e. function results.
For addresses: all projections of a useful address should be useful.
This has special support:
`DifferentiableActivityInfo::propagateUsefulThroughAddress`.
Previously:
- Usefulness was propagated by iterating through all instructions in
post-dominance order. This is not efficient because irrelevant instructions
may be visited.
- For useful addresses, `propagateUsefulThroughAddress` propagated
usefulness one step to projections, but not recursively to users of the
projections. This caused some values to incorrectly not be marked useful.
Now:
- Usefulness is propagated by following use-def chains, starting from
dependent variables (function results). This is handled by the
following helpers:
- `setUsefulAndPropagateToOperands(SILValue, unsigned)`: marks a value as
useful and recursively propagates usefulness through defining instruction
operands and basic block argument incoming values.
- `propagateUseful(SILInstruction *inst, unsigned)`: propagates usefulness
to the operands of the given instruction.
- `DifferentiableActivityInfo::propagateUsefulThroughAddress` now calls
`propagateUseful` to propagate usefulness recursively through users'
operands.
Effects:
- More values are now (correctly) marked as useful, affecting
non-differentiability diagnostics for active enum values (TF-956) and for-in
loops (TF-957). Both have room for improvement.
Resolves control flow differentiation correctness issue: TF-954.
dan-zheng
force-pushed
the
autodiff-fix-activity-analysis
branch
from
05e0b86 to
af915c0
Compare
|
@swift-ci Please test tensorflow |
rxwei
approved these changes
Nov 13, 2019
|
@swift-ci Please test tensorflow |
dan-zheng
added a commit
to dan-zheng/swift
that referenced
this pull request
Nov 16, 2019
Hoist activity marking visited value set out of loop over original bbs. This is safe because bbs directly start with dominator bbs's active values. Visit bb arguments for activity marking. This was accidentally deleted in swiftlang#28225. Re-adding the logic doesn't seem to affect any tests.
dan-zheng
added a commit
that referenced
this pull request
Nov 16, 2019
Hoist activity marking visited value set out of loop over original bbs. This is safe because bbs directly start with dominator bbs's active values. Visit bb arguments for activity marking. This was accidentally deleted in #28225. Re-adding the logic doesn't seem to affect any tests.
bgogul
pushed a commit
that referenced
this pull request
Nov 19, 2019
Useful values are those that contribute to (specific) dependent variables,
i.e. function results.
For addresses: all projections of a useful address should be useful.
This has special support:
`DifferentiableActivityInfo::propagateUsefulThroughAddress`.
Previously:
- Usefulness was propagated by iterating through all instructions in
post-dominance order. This is not efficient because irrelevant instructions
may be visited.
- For useful addresses, `propagateUsefulThroughAddress` propagated
usefulness one step to projections, but not recursively to users of the
projections. This caused some values to incorrectly not be marked useful.
Now:
- Usefulness is propagated by following use-def chains, starting from
dependent variables (function results). This is handled by the
following helpers:
- `setUsefulAndPropagateToOperands(SILValue, unsigned)`: marks a value as
useful and recursively propagates usefulness through defining instruction
operands and basic block argument incoming values.
- `propagateUseful(SILInstruction *inst, unsigned)`: propagates usefulness
to the operands of the given instruction.
- `DifferentiableActivityInfo::propagateUsefulThroughAddress` now calls
`propagateUseful` to propagate usefulness recursively through users'
operands.
Effects:
- More values are now (correctly) marked as useful, affecting
non-differentiability diagnostics for active enum values (TF-956) and for-in
loops (TF-957). Both have room for improvement.
Resolves control flow differentiation correctness issue: TF-954.
dan-zheng
added a commit
that referenced
this pull request
Nov 21, 2019
Hoist activity marking visited value set out of loop over original bbs. This is safe because bbs directly start with dominator bbs's active values. Visit bb arguments for activity marking. This was accidentally deleted in #28225. Re-adding the logic doesn't seem to affect any tests.
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.
Useful values are those that contribute to (specific) dependent variables,
i.e. function results.
For addresses: all projections of a useful address should be useful.
This has special support:
DifferentiableActivityInfo::propagateUsefulThroughAddress.Previously:
post-dominance order. This is not efficient because irrelevant instructions
may be visited.
propagateUsefulThroughAddresspropagatedusefulness one step to projections, but not recursively to users of the
projections. This caused some values to incorrectly not be marked useful.
Now:
dependent variables (function results). This is handled by the
following helpers:
setUsefulAndPropagateToOperands(SILValue, unsigned): marks a value asuseful and recursively propagates usefulness through defining instruction
operands and basic block argument incoming values.
propagateUseful(SILInstruction *inst, unsigned): propagates usefulnessto the operands of the given instruction.
DifferentiableActivityInfo::propagateUsefulThroughAddressnow callspropagateUsefulto propagate usefulness recursively through users'operands.
Effects:
non-differentiability diagnostics for active enum values (TF-956) and for-in
loops (TF-957). Both have room for improvement.
Resolves control flow differentiation correctness issue: TF-954.