TempRValueOptPass: accept try_apply as user of the temporary object

[eeckstein]

So far we only handled apply, but not try_apply.

Also: some small cosmetic changes

[eeckstein]

@swift-ci benchmark

[eeckstein]

@swift-ci test

[swift-ci]

Performance: -O

Regression	OLD	NEW	DELTA	RATIO
SubstringEquatable	418	541	+29.4%	0.77x
ChainedFilterMap	675	792	+17.3%	0.85x (?)
MapReduceLazyCollectionShort	26	28	+7.7%	0.93x (?)
Improvement	OLD	NEW	DELTA	RATIO
ArrayAppendGenericStructs	670	590	-11.9%	1.14x (?)
ArrayAppendUTF16Substring	11340	10368	-8.6%	1.09x

Code size: -O

Regression	OLD	NEW	DELTA	RATIO
ChainedFilterMap.o	2661	3445	+29.5%	0.77x
RangeOverlaps.o	9818	10213	+4.0%	0.96x
CaptureProp.o	899	931	+3.6%	0.97x
Improvement	OLD	NEW	DELTA	RATIO
ReduceInto.o	16509	15346	-7.0%	1.08x
DictionaryCompactMapValues.o	17633	17441	-1.1%	1.01x

Performance: -Osize

Regression	OLD	NEW	DELTA	RATIO
MapReduceLazyCollectionShort	26	47	+80.8%	0.55x
SubstringEquatable	424	534	+25.9%	0.79x
SuffixCountableRange	4	5	+25.0%	0.80x (?)
ArrayAppendLazyMap	580	680	+17.2%	0.85x (?)
SortIntPyramid	440	475	+8.0%	0.93x (?)
Improvement	OLD	NEW	DELTA	RATIO
MapReduceAnyCollectionShort	1590	1470	-7.5%	1.08x (?)

Code size: -Osize

Regression	OLD	NEW	DELTA	RATIO
SequenceAlgos.o	19615	21023	+7.2%	0.93x
ChainedFilterMap.o	3301	3381	+2.4%	0.98x
Improvement	OLD	NEW	DELTA	RATIO
ReduceInto.o	9581	9028	-5.8%	1.06x
DictionaryKeysContains.o	7503	7271	-3.1%	1.03x
DictionaryCompactMapValues.o	13119	12743	-2.9%	1.03x
SortLargeExistentials.o	21020	20748	-1.3%	1.01x

Performance: -Onone

Regression	OLD	NEW	DELTA	RATIO
DataSubscriptMedium	58	64	+10.3%	0.91x (?)
Improvement	OLD	NEW	DELTA	RATIO
IterateData	3566	3025	-15.2%	1.18x
ArrayOfPOD	728	650	-10.7%	1.12x (?)
Breadcrumbs.MutatedUTF16ToIdx.ASCII	15	14	-6.7%	1.07x (?)

Code size: -swiftlibs

How to read the data

The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview

  Model Name: Mac mini
  Model Identifier: Macmini8,1
  Processor Name: Intel Core i7
  Processor Speed: 3.2 GHz
  Number of Processors: 1
  Total Number of Cores: 6
  L2 Cache (per Core): 256 KB
  L3 Cache: 12 MB
  Memory: 64 GB

[gottesmm]

Can this ever work if something isn't a full apply site? If not, can you make this a full apply site?

[eeckstein]

Yeah, that would probably be more descriptive. But using an ApplySite does not hurt here, because the switch case already selects only full apply sites: ApplyInst and TryApplyInst.

[gottesmm]

Why did you remove the rest of this stuff from this PR?

[eeckstein]

I added more detailed comment in the commit message (I should have copied that to the PR description):

1. WitnessMethodInst doesn't need to be handled as use because it's operand is a typedependent operand, which we exclude anyway when looking at the uses.
2. Replace a list of handled instructions with a default value in the switch. It's easy to forget to add an instruction here. So it's safer to have a default behavior.

I hope that answers your question

[gottesmm]

I think this is the wrong approach. It will make it so that we by mistake handle things that we may not intend to. That is a large problem with this pass.

Thought: why not instead make it so that the default in asserts builds asserts, but in non-asserts builds has the other behavior?

[eeckstein]

I don't agree. We want to handle a few instructions in a special way, all other instructions the same. So IMO, using "default" is the right choice here.

[gottesmm]

The statement "we want to handle a few instruction in a special way, all other instructions the same" is incorrect.

When this pass was written we were pattern matching a specific semantic codegen pattern, so the code was written this way by @atrick to make sure that we /only/ pattern matched that pattern and if we hit a pattern by mistake that was not anticipated, we got a nice error from the compiler so we knew to update this code.

With this change, we will not get that error and instead will start silently accepting such new code patterns, allowing for the introduction of bugs and a reduction in the quality and correctness of the compiler.

[atrick]

I don't really care about this particular case--it seems fairly hard to break. But Michael is right that the original intention was for the optimization to be self-checking.

When correctness is at stake, it's generally important to list both the specially handled instructions and the "known-safe" instructions. There are so many variations of SIL operations, and we're continually adding more, so new instructions might come along that need special handling. (The previous bug was that an instruction was specially handled, but handled incorrectly).

Listing known-safe cases also communicates to future maintainers which cases were deliberately handled vs. which cases were forgotten. It helps me when reading the code to know what all the expected cases are without taking time to step through the code.

There is always the tradeoff between aggressively asserting in optimization passes vs. falling back to something that is probably safe. The LLVM/Swift approach has generally been to aggressively assert. It means many more bug reports early on but fewer hard-to-reproduce bugs later in the produce lifecycle.

[atrick]

LGTM: I don't know why we weren't handling try_apply before.