Skip to content

TempRValueOptPass: accept try_apply as user of the temporary object #27721

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

Merged
merged 2 commits into from
Oct 16, 2019
Merged

TempRValueOptPass: accept try_apply as user of the temporary object #27721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aa0d60a
TempRValueOptPass: some small cosmetic changes
eeckstein Oct 16, 2019
e7ddae6
TempRValueOptPass: accept try_apply as user of the temporary object
eeckstein Oct 16, 2019
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
15 changes: 3 additions & 12 deletions lib/SILOptimizer/Transforms/CopyForwarding.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1645,7 +1645,8 @@ bool TempRValueOptPass::collectLoads(
<< *user);
return false;

case SILInstructionKind::ApplyInst: {
case SILInstructionKind::ApplyInst:
case SILInstructionKind::TryApplyInst: {
ApplySite apply(user);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


// Check if the function can just read from userOp.
Expand Down Expand Up @@ -1714,12 +1715,10 @@ bool TempRValueOptPass::collectLoads(

case SILInstructionKind::LoadInst:
case SILInstructionKind::LoadBorrowInst:
case SILInstructionKind::WitnessMethodInst: {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

// Loads are the end of the data flow chain. The users of the load can't
// access the temporary storage.
loadInsts.insert(user);
return true;
}

case SILInstructionKind::CopyAddrInst: {
// copy_addr which read from the temporary are like loads.
Expand Down Expand Up @@ -1834,17 +1833,9 @@ bool TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) {
use->set(copyInst->getSrc());
break;
}
case SILInstructionKind::StructElementAddrInst:
case SILInstructionKind::TupleElementAddrInst:
case SILInstructionKind::LoadInst:
case SILInstructionKind::LoadBorrowInst:
case SILInstructionKind::ApplyInst:
case SILInstructionKind::OpenExistentialAddrInst:
default:
use->set(copyInst->getSrc());
break;

default:
llvm_unreachable("unhandled instruction");
}
}
tempObj->eraseFromParent();
Expand Down
22 changes: 22 additions & 0 deletions test/SILOptimizer/temp_rvalue_opt.sil
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ bb0(%0 : $*Klass, %1 : $*Klass):
return %9999 : $()
}

sil @throwing_function : $@convention(thin) (@in_guaranteed Klass) -> ((), @error Error)

///////////
// Tests //
///////////
Expand Down Expand Up @@ -383,6 +385,26 @@ bb0(%0 : $*Klass):
return %9 : $()
}

// CHECK-LABEL: sil @try_apply_argument : $@convention(thin) (@inout Klass) -> () {
// CHECK-NOT: copy_addr
// CHECK: try_apply {{%[0-9]+}}(%0)
// CHECK: } // end sil function 'try_apply_argument'
sil @try_apply_argument : $@convention(thin) (@inout Klass) -> () {
bb0(%0 : $*Klass):
%1 = alloc_stack $Klass
copy_addr %0 to [initialization] %1 : $*Klass
%5 = function_ref @throwing_function : $@convention(thin) (@in_guaranteed Klass) -> ((), @error Error)
try_apply %5(%1) : $@convention(thin) (@in_guaranteed Klass) -> ((), @error Error), normal bb1, error bb2
bb1(%r : $()):
br bb3
bb2(%e : $Error):
br bb3
bb3:
destroy_addr %1 : $*Klass
dealloc_stack %1 : $*Klass
%9 = tuple ()
return %9 : $()
}

// Make sure that we can eliminate temporaries passed via a temporary rvalue to
// an @in_guaranteed function.
Expand Down