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.

Sign up for GitHub

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

Eliminate tuple allocations in branching let binding rhs #11407

Merged
merged 6 commits into from
Jun 16, 2021
Merged

Eliminate tuple allocations in branching let binding rhs #11407

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e6c8a2d
Eliminate tuple allocations in branching let binding rhs
kerams Apr 10, 2021
16bac85
Add more tuple elimination tests
kerams Apr 13, 2021
b532523
Address issues
kerams May 7, 2021
0c37bbe
Refactor
kerams May 10, 2021
3ea5579
Refactor
kerams May 12, 2021
af36a78
Fix condition
kerams May 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/fsharp/Optimizer.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1278,7 +1278,7 @@ let ValueOfExpr expr =
ConstExprValue(0, expr)
else UnknownValue

let IsMutableStructuralBindingForTupleElement (vref: ValRef) = vref.DisplayName.EndsWith suffixForTupleElementAssignmentTarget
let IsMutableStructuralBindingForTupleElement (vref: ValRef) = vref.IsCompilerGenerated && vref.DisplayName.EndsWith suffixForTupleElementAssignmentTarget

//-------------------------------------------------------------------------
// Dead binding elimination
Expand Down Expand Up @@ -2586,7 +2586,7 @@ and AddValEqualityInfo g m (v: ValRef) info =
// when their address is passed to the method call. Another exception are mutable variables
// created for tuple elimination in branching tuple bindings because they are assigned to
// exactly once.
if v.IsMutable && not (v.IsCompilerGenerated && (v.DisplayName.StartsWith(PrettyNaming.outArgCompilerGeneratedName) || IsMutableStructuralBindingForTupleElement v)) then
if v.IsMutable && not (IsMutableStructuralBindingForTupleElement v) && not (v.IsCompilerGenerated && v.DisplayName.StartsWith(PrettyNaming.outArgCompilerGeneratedName)) then
Copy link
Contributor

Choose a reason for hiding this comment

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

the not a && not b is irking me, does inverting the conditionals / branch looks better?

Also wondering about repeated calls to v.IsCompilerGenerated that now occurs (one is introduced, unless mistaken).

As you are stress testing the inliner and bool expression optimizer, maybe you should also stress test the final reviewer by rewriting the whole condition from scratch to make it more readable 🙂.

Suggestion, since both current branches are very small expressions that could be copied:

In the comment, it says

an exception can be made for outArg values...

this would be one simple if condition, then it says

Another exception are mutable variables
created for tuple elimination in branching tuple bindings because they are assigned to
exactly once.

This would be another one, the final case is just returning the default case.

Sorry if bikeshedding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does inverting the conditionals / branch looks better

not (a || b)? Hm, I don't feel strongly either way.

Also wondering about repeated calls to v.IsCompilerGenerated

I know, but it needs to be a part of IsMutableStructuralBindingForTupleElement, otherwise If the caller does not also check IsCompilerGenerated, then it could lead to bugs.

So what you're suggesting at the end is this?

if v.IsMutable && not (IsMutableStructuralBindingForTupleElement v) then
    info
elif v.IsMutable && not (v.IsCompilerGenerated && v.DisplayName.StartsWith(PrettyNaming.outArgCompilerGeneratedName)) then
    info
else
    { info with Info = MakeValueInfoForValue g m v info.Info }

Copy link
Contributor

Choose a reason for hiding this comment

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

@kerams, thanks a lot for the feedback.

not (a || b)? Hm, I don't feel strongly either way.

if that's the end result, the best (to me) would be if a || b then and inverting the branches expressions then (I don't like "not" for complex conditions, the same way I don't like isNull, I'd prefer isNotNull and not using "not" 🙂).

I know, but it needs to be a part of IsMutableStructuralBindingForTupleElement, otherwise If the caller does not also check IsCompilerGenerated, then it could lead to bugs.

Understood, it fits well with when I was proposing to extract a function, and it makes sense to have the redundant check to properly encapsulate the assumption.

It may also surface as a comment if you can expand on the deeper reason, to avoid someone trying to remove duplicate check to do it hastily.

So what you're suggesting at the end is this?

for the unsuspecting next maintainer dealing with this part (which seems sensitive for tuple elision, but it, hmm, escapes me how it actually works), I feel your proposed edit, with maybe a re-interleaving of the keywords (such as "outArg values", "temp mutable for tuples that can be elided") from the larger text above, would do a great job at improving the very expansive conditional in the code, and shouldn't add runtime cost, just a duplicate branch with info.

So I see potential small wins for the clarity of the code, if this is not bikeshedding and you feel it makes it easier for next person following your journey, I wish to understand better the flow of Val objects and do the work you do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

which seems sensitive for tuple elision, but it, hmm, escapes me how it actually works

let a, b =
    if System.Diagnostics.Debugger.IsAttached then
        1, 3
    else
        3, 4
a + b

enters TryRewriteBranchingTupleBinding in the form of

let patternInput = if System.Diagnostics.Debugger.IsAttached then 1, 3 else 3, 4
let b = patternInput.Item2
let a = patternInput.Item1
a + b

and exits as

let mutable patternInput_0_tupleElem = null
let mutable patternInput_1_tupleElem = null

let patternInput =
    if System.Diagnostics.Debugger.IsAttached then
        patternInput_0_tupleElem <- 1
        patternInput_1_tupleElem <- 3
    else
        patternInput_0_tupleElem <- 3
        patternInput_1_tupleElem <- 4

    patternInput_0_tupleElem, patternInput_1_tupleElem

let b = patternInput.Item2
let a = patternInput.Item1
a + b

As you can see, there's still a let binding with a tuple (I copied that part from ExpandStructuralBindingRaw, the 4 lines in the else branch). It's only further on in OptimizeExprOpReductionsAfter and TryOptimizeTupleFieldGet that this tuple is eliminated. If you step through the code, you'll realize it's only possible when the equivalence of patternInput_0_tupleElem and patternInput.Item1 is established based on the former's ExprValueInfo. Had I not changed AddValEqualityInfo to account for this mutable tupleElem, there'd be no equality information (because normally you can't, for obvious reasons, equate a mutable variable with another one) and the result would look like:

public static int z()
{
	int item;
	int item2;
	if (Debugger.IsAttached)
	{
		item = 1;
		item2 = 3;
	}
	else
	{
		item = 3;
		item2 = 4;
	}
	Tuple<int, int> tuple = new Tuple<int, int>(item, item2);
	return tuple.Item1 + tuple.Item2;
}

If, on the other hand, I'd used a regular local in MakeMutableStructuralBindingForTupleElement instead of mkMutableCompGenLocal, patternInput_0_tupleElem would be equated with that initial null (0) forever, and the whole function would through a series of inlining and eliminations be reduced to this:

public static int z()
{
	if (Debugger.IsAttached)
	{
	}
	return 0 + 0;
}

info
else
{info with Info= MakeValueInfoForValue g m v info.Info}
Expand Down