[DebugInfo] Fix loss of variables at -Onone

[Snowy1803]

Fix the loss of variables in 3 mandatory SIL passes:

• AllocBoxToStack was setting the wrong scope on the created alloc_stack in some cases.
• LoadableByAddress was not salvaging removed loads, and was using the wrong scope in some cases.
• AllocStackHoisting was only keeping debug info for one of the variables when merging alloc_stacks of the same type.

This fixes all real losses of variables in the standard library when compiling at Onone. It also affects the number of variables available with optimizations on.

[Snowy1803]

@swift-ci please test

[adrian-prantl]

this change LGTM

[atrick]

@Snowy1803 @adrian-prantl SILBuilderWithScope is supposed to be the correct API. We want to migrate all uses of the SILBuilder to it and replace SILBuilder with it rather than having two types.

The idea that SILBuilderWithScope should not be called on certain instruction types is not maintainable. Why can't we just fix it to do the right thing for these instructions?

[atrick]

By pure chance, I just reviewed code that does this
SILBuilderWithScope builder(ai->getNextInstruction()
So, the solution that this PR takes doesn't seem like it can possibly work.

[Snowy1803]

The idea that SILBuilderWithScope should not be called on certain instruction types is not maintainable. Why can't we just fix it to do the right thing for these instructions?

@atrick SILBuilderWithScope skips over debug values, because meta instructions would not exist in assembly.

When we know that we have a debug_value, and want to replace it with a new one -> we want to take the debug_value's scope
When we are moving code somewhere, on an unknown instruction, we want to ignore any alloc_stack or debug_value that might be there, and so we can use SILBuilderWithScope

[atrick]

@Snowy1803 that makes sense. If those are the rules, then they should at least be specified in SILBuilderWithScope class-level comments. Those comments currently seem inconsistent with the rule. Ultimately, we need to distinguish between an arbitrary insertion point vs. instruction replacement/expansion/lowering. And instruction replacement should let you insert either before or after the original.

[eeckstein]

How about Builder in the SwiftCompilerSources? I think it correlates as follows:

1. SILBuilderWithScope -> Builder.init(before: Instruction) or Builder.init(after: Instruction): The location (which includes the SILDebugScope) is taken from the insertion point
2. SILBuilder -> Builder.init(before: Instruction, location: Location) or Builder.init(after: Instruction, location: Location) (again, location includes the scope)

The one piece which is missing is the skipping of "meta instructions". Should we add this to the first kind of Builder initializers?

[atrick]

@eeckstein Yeah. I think Emil is saying that these APIs should normally skip "meta instructions"
Builder.init(before: Instruction) or Builder.init(after: Instruction)

But maybe we should have:
Builder.init(replacing: Instruction) or Builder.init(inherit: Instruction)
which inherits location and scope directly from the instruction being replaced.
Hopefully it always works to insert before the replacement. I haven't audited the code to see if it would make sense though.

[Snowy1803]

Yes, I think it would make sense to have a Builder.init(replacing: Instruction). Inserting before the replacement should work. The only difference is for meta instructions, which don't really affect the program, so inserting before the replaced instruction should never be a problem for those.

[eeckstein]

Builder.init(before: Instruction) and Builder.init(after: Instruction) already inherit the instruction's location and scope.
What would be different with Builder.init(replacing: Instruction)?

[Snowy1803]

Builder.init(before: Instruction) and Builder.init(after: Instruction) should skip meta instructions to find a location and scope, just like SILBuilderWithScope in C++. Builder.init(replacing: Instruction) would be equivalent to the current Builder.init(before: Instruction), which doesn't skip meta instructions.

[eeckstein]

Builder.init(before: Instruction) and Builder.init(after: Instruction) should skip meta instructions to find a location and scope

Makes sense. Can you open a PR for this?

Builder.init(replacing: Instruction) would be equivalent to the current Builder.init(before: Instruction), which doesn't skip meta instructions.

"replacing" is confusing because it doesn't tell you if the insertion point is before or after the instruction and it's the user's responsibility to actually delete the instruction.

If you want this behavior, you can do let b = Builder.init(before: i, location: i.location), right?

[Snowy1803]

If you want this behavior, you can do let b = Builder.init(before: i, location: i.location), right?

Yes, that's what we currently do in C++

I will open a PR for this

[atrick]

I think Builder.init(replacing:) is the API that makes the most sense, but if there's a serious concern about the name, then it can be Builder.init(inheriting:). Inserting before the thing you're replacing is the only thing that makes sense and will work in general.

Note that the complete semantics of an API do not need to be in the name. It's perfectly fine to specify behavior in the API coment.