[IRGen] Avoid generating mergeable traps #14729

vedantk · 2018-02-20T02:11:41Z

Factor out and reuse logic in the lowering of CondFailInst to emit
non-mergeable traps, everywhere we emit traps. This should address a
debugging quality issue with ambiguous ud2 instructions.

rdar://32772768

Factor out and reuse logic in the lowering of CondFailInst to emit non-mergeable traps, everywhere we emit traps. This should address a debugging quality issue with ambiguous ud2 instructions. rdar://32772768

vedantk · 2018-02-20T02:11:54Z

@swift-ci smoke test

vedantk · 2018-02-20T02:12:38Z

@swift-ci Please smoke test compiler performance

vedantk · 2018-02-20T02:15:14Z

Ah, I don't think this is a sure shot to fix the ambiguous ud2 issue for good. We should look at unreachable instructions too.

Edit: At least, test/CodeGen/X86/empty-function.ll in llvm indicates that we may generate ud2/int3 without a call to llvm.trap().

swift-ci · 2018-02-20T03:42:56Z

Build comment file:

Summary for master smoketest

Unexpected test results, stats may be off for 3

No regressions above thresholds

Debug
- debug brief
- debug detailed
Release
- release brief
- release detailed

Debug

debug brief

Regressed (0)

name	old	new	delta	delta_pct

Improved (0)

name	old	new	delta	delta_pct

Unchanged (delta < 1.0% or delta < 100.0ms) (2)

name	old	new	delta	delta_pct
LLVM.NumLLVMBytesOutput	43,736,712	43,595,264	-141,448	-0.32%
time.swift-driver.wall	60.9s	60.6s	-254.9ms	-0.42%

debug detailed

Regressed (0)

name	old	new	delta	delta_pct

Improved (2)

name	old	new	delta	delta_pct
AST.NumUsedConformances	4,235	4,190	-45	-1.06% ✅
Sema.NumConformancesDeserialized	183,910	181,804	-2,106	-1.15% ✅

Unchanged (delta < 1.0% or delta < 100.0ms) (21)

name	old	new	delta	delta_pct
AST.NumImportedExternalDefinitions	71,509	71,295	-214	-0.3%
AST.NumLoadedModules	10,881	10,859	-22	-0.2%
AST.NumTotalClangImportedEntities	198,692	198,218	-474	-0.24%
IRModule.NumIRBasicBlocks	110,414	109,850	-564	-0.51%
IRModule.NumIRFunctions	63,104	62,830	-274	-0.43%
IRModule.NumIRGlobals	83,020	82,747	-273	-0.33%
IRModule.NumIRInsts	1,288,017	1,282,471	-5,546	-0.43%
IRModule.NumIRValueSymbols	126,750	126,250	-500	-0.39%
LLVM.NumLLVMBytesOutput	43,736,712	43,595,264	-141,448	-0.32%
SILModule.NumSILGenFunctions	77,267	76,944	-323	-0.42%
SILModule.NumSILOptFunctions	53,709	53,325	-384	-0.71%
Sema.NumConstraintScopes	480,809	478,219	-2,590	-0.54%
Sema.NumDeclsDeserialized	1,391,318	1,381,330	-9,988	-0.72%
Sema.NumDeclsValidated	45,173	44,937	-236	-0.52%
Sema.NumFunctionsTypechecked	41,876	41,790	-86	-0.21%
Sema.NumGenericSignatureBuilders	64,509	64,307	-202	-0.31%
Sema.NumLazyGenericEnvironments	251,644	249,945	-1,699	-0.68%
Sema.NumLazyGenericEnvironmentsLoaded	29,527	29,405	-122	-0.41%
Sema.NumLazyIterableDeclContexts	242,187	241,149	-1,038	-0.43%
Sema.NumTypesDeserialized	1,448,231	1,437,747	-10,484	-0.72%
Sema.NumTypesValidated	189,072	188,689	-383	-0.2%

Release

release brief

Regressed (0)

name	old	new	delta	delta_pct

Improved (0)

name	old	new	delta	delta_pct

Unchanged (delta < 1.0% or delta < 100.0ms) (2)

name	old	new	delta	delta_pct
LLVM.NumLLVMBytesOutput	40,182,960	40,182,344	-616	-0.0%
time.swift-driver.wall	101.1s	100.9s	-137.3ms	-0.14%

release detailed

Regressed (0)

name	old	new	delta	delta_pct

Improved (0)

name	old	new	delta	delta_pct

Unchanged (delta < 1.0% or delta < 100.0ms) (23)

name	old	new	delta	delta_pct
AST.NumImportedExternalDefinitions	10,387	10,387	0	0.0%
AST.NumLoadedModules	365	365	0	0.0%
AST.NumTotalClangImportedEntities	29,955	29,955	0	0.0%
AST.NumUsedConformances	4,242	4,242	0	0.0%
IRModule.NumIRBasicBlocks	88,996	88,986	-10	-0.01%
IRModule.NumIRFunctions	40,405	40,411	6	0.01%
IRModule.NumIRGlobals	47,475	47,475	0	0.0%
IRModule.NumIRInsts	860,619	860,603	-16	-0.0%
IRModule.NumIRValueSymbols	83,029	83,035	6	0.01%
LLVM.NumLLVMBytesOutput	40,182,960	40,182,344	-616	-0.0%
SILModule.NumSILGenFunctions	21,816	21,816	0	0.0%
SILModule.NumSILOptFunctions	28,263	28,263	0	0.0%
Sema.NumConformancesDeserialized	94,768	94,768	0	0.0%
Sema.NumConstraintScopes	449,767	449,767	0	0.0%
Sema.NumDeclsDeserialized	200,647	200,647	0	0.0%
Sema.NumDeclsValidated	28,395	28,395	0	0.0%
Sema.NumFunctionsTypechecked	11,257	11,257	0	0.0%
Sema.NumGenericSignatureBuilders	7,694	7,694	0	0.0%
Sema.NumLazyGenericEnvironments	32,787	32,787	0	0.0%
Sema.NumLazyGenericEnvironmentsLoaded	3,871	3,871	0	0.0%
Sema.NumLazyIterableDeclContexts	20,706	20,706	0	0.0%
Sema.NumTypesDeserialized	233,424	233,424	0	0.0%
Sema.NumTypesValidated	56,801	56,801	0	0.0%

jckarter · 2018-02-20T17:25:55Z

I feel like the ultimate solution to this belongs at the LLVM level, with some kind of llvm.nonmergeable.trap intrinsic. The asm barriers have the unfortunate side effect of impeding other LLVM-level optimizations we want.

vedantk · 2018-02-20T19:20:03Z

I feel like the ultimate solution to this belongs at the LLVM level, with some kind of llvm.nonmergeable.trap intrinsic. The asm barriers have the unfortunate side effect of impeding other LLVM-level optimizations we want.

It would be worth having a non-mergeable trap intrinsic just to avoid inline-asm hacks in the frontend.

Out of curiosity though, which optimizations would we unblock with such an intrinsic? At a quick glance I see that GVN and SimplifyCFG treat inline asm instructions as barriers. Both of these passes would have to treat non-mergeable traps as barriers too.

rudkx

LGTM, but yes it would be great if we had an LLVM IR trap that was guaranteed to remain unique so that we can post-mortem debug code and know how we reached the trap.

adrian-prantl

Awesome!

adrian-prantl · 2018-02-21T16:38:17Z

lib/IRGen/IRGenFunction.cpp

+
+void IRGenFunction::emitTrap(bool EmitUnreachable) {
+  if (IGM.IRGen.Opts.shouldOptimize()) {
+    // Emit unique side-effecting inline asm calls in order to eliminate


Shouldn't this be the Doxygen comment for the function?

IMO this describes the implementation details at too low a level. The doxygen comment I added in the header file describes the high-level effect more succinctly.

adrian-prantl · 2018-02-21T16:40:43Z

lib/IRGen/IRGenSIL.cpp

@@ -3851,9 +3850,7 @@ static bool hasReferenceSemantics(IRGenSILFunction &IGF,
 static llvm::Value *emitIsUnique(IRGenSILFunction &IGF, SILValue operand,
                                 SourceLoc loc, bool checkPinned) {
  if (!hasReferenceSemantics(IGF, operand->getType())) {
-    llvm::Function *trapIntrinsic = llvm::Intrinsic::getDeclaration(
-      &IGF.IGM.Module, llvm::Intrinsic::ID::trap);
-    IGF.Builder.CreateCall(trapIntrinsic, {});


Is there a way we could prevent this from being called by accident in new code, maybe by hiding access to that particular intrinsic behind a private method? It may not be feasible because it is just modeled as a call...

I think it's possible to interpose a custom builder over llvm::IRBuilder, e.g clang does it. And if we can do that, we can prevent calls to ID::trap from being created. I'll look into it.

PR swiftlang#14729 made more calls to llvm.trap() non-mergeable. This follow-up adds asserts to IRBuilder which make it harder to accidentally introduce mergeable calls to llvm.trap() in the future. The newly-added assertions exposed an issue in GenBuiltin while compiling parts of the stdlib. This PR fixes the issue. Suggested by Adrian Prantl! rdar://32772768

PR #14729 made more calls to llvm.trap() non-mergeable. This follow-up adds asserts to IRBuilder which make it harder to accidentally introduce mergeable calls to llvm.trap() in the future. The newly-added assertions exposed an issue in GenBuiltin while compiling parts of the stdlib. This PR fixes the issue. Suggested by Adrian Prantl! rdar://32772768

[IRGen] Avoid generating mergeable traps

ac0bf37

Factor out and reuse logic in the lowering of CondFailInst to emit non-mergeable traps, everywhere we emit traps. This should address a debugging quality issue with ambiguous ud2 instructions. rdar://32772768

vedantk requested review from rudkx and adrian-prantl February 20, 2018 02:11

rudkx approved these changes Feb 20, 2018

View reviewed changes

vedantk merged commit 132bb19 into swiftlang:master Feb 20, 2018

vedantk deleted the traps branch February 20, 2018 21:40

adrian-prantl reviewed Feb 21, 2018

View reviewed changes

vedantk mentioned this pull request Feb 24, 2018

[IRGen] Prevent mergeable traps from being created #14815

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[IRGen] Avoid generating mergeable traps #14729

[IRGen] Avoid generating mergeable traps #14729

Uh oh!

vedantk commented Feb 20, 2018

Uh oh!

vedantk commented Feb 20, 2018

Uh oh!

vedantk commented Feb 20, 2018

Uh oh!

vedantk commented Feb 20, 2018 •

edited

Loading

Uh oh!

swift-ci commented Feb 20, 2018

Debug

debug brief

debug detailed

Release

release brief

release detailed

Uh oh!

jckarter commented Feb 20, 2018

Uh oh!

vedantk commented Feb 20, 2018

Uh oh!

rudkx left a comment

Uh oh!

adrian-prantl left a comment

Uh oh!

adrian-prantl Feb 21, 2018

Uh oh!

vedantk Feb 21, 2018

Uh oh!

adrian-prantl Feb 21, 2018

Uh oh!

vedantk Feb 21, 2018

Uh oh!

Uh oh!

[IRGen] Avoid generating mergeable traps #14729

[IRGen] Avoid generating mergeable traps #14729

Uh oh!

Conversation

vedantk commented Feb 20, 2018

Uh oh!

vedantk commented Feb 20, 2018

Uh oh!

vedantk commented Feb 20, 2018

Uh oh!

vedantk commented Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swift-ci commented Feb 20, 2018

Build comment file:

Summary for master smoketest

Debug

debug brief

debug detailed

Release

release brief

release detailed

Uh oh!

jckarter commented Feb 20, 2018

Uh oh!

vedantk commented Feb 20, 2018

Uh oh!

rudkx left a comment

Choose a reason for hiding this comment

Uh oh!

adrian-prantl left a comment

Choose a reason for hiding this comment

Uh oh!

adrian-prantl Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

vedantk Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

adrian-prantl Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

vedantk Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vedantk commented Feb 20, 2018 •

edited

Loading