Skip to content

Commit a7e7f5d

Browse files
devincoughlindevincoughlin
authored
Merge pull request #35742 from guitard0g/guitard0g/animationSignpostConstEvalFix54
[5.4] Fix OSLogOptimization pass to mark array.finalize_intrinsic as not foldable
2 parents 44d9d8b + a56bb11 commit a7e7f5d

File tree

6 files changed

+184
-2
lines changed

6 files changed

+184
-2
lines changed

‎include/swift/SIL/SILConstants.h‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -604,6 +604,11 @@ class SymbolicValue {
604604
/// version. This only works for valid constants.
605605
SymbolicValue cloneInto(SymbolicValueAllocator &allocator) const;
606606

607+
/// Check that all nested SymbolicValues are constant. Symbolic values such as arrays,
608+
/// aggregates and pointers can contain non-constant symbolic values, when instructions
609+
/// are skipped during evaluation.
610+
bool containsOnlyConstants() const;
611+
607612
void print(llvm::raw_ostream &os, unsigned indent = 0) const;
608613
void dump() const;
609614
};

‎lib/SIL/IR/SILConstants.cpp‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,52 @@ SymbolicValue::cloneInto(SymbolicValueAllocator &allocator) const {
260260
llvm_unreachable("covered switch");
261261
}
262262

263+
bool SymbolicValue::containsOnlyConstants() const {
264+
if (!isConstant())
265+
return false;
266+
267+
auto thisRK = representationKind;
268+
switch (thisRK) {
269+
case RK_UninitMemory:
270+
case RK_Unknown:
271+
case RK_Metatype:
272+
case RK_Function:
273+
case RK_Enum:
274+
case RK_IntegerInline:
275+
case RK_Integer:
276+
case RK_String:
277+
case RK_Closure:
278+
return true;
279+
case RK_Aggregate: {
280+
auto elts = getAggregateMembers();
281+
for (auto elt : elts)
282+
if (!elt.containsOnlyConstants())
283+
return false;
284+
return true;
285+
}
286+
case RK_EnumWithPayload: {
287+
return getEnumPayloadValue().containsOnlyConstants();
288+
}
289+
case RK_DirectAddress:
290+
case RK_DerivedAddress: {
291+
auto *memObject = getAddressValueMemoryObject();
292+
return memObject->getValue().containsOnlyConstants();
293+
}
294+
case RK_ArrayStorage: {
295+
CanType elementType;
296+
ArrayRef<SymbolicValue> elts = getStoredElements(elementType);
297+
for (auto elt : elts)
298+
if (!elt.containsOnlyConstants())
299+
return false;
300+
return true;
301+
}
302+
case RK_Array: {
303+
return getStorageOfArray().containsOnlyConstants();
304+
}
305+
}
306+
llvm_unreachable("covered switch");
307+
}
308+
263309
//===----------------------------------------------------------------------===//
264310
// SymbolicValueMemoryObject implementation
265311
//===----------------------------------------------------------------------===//

‎lib/SILOptimizer/Mandatory/OSLogOptimization.cpp‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -322,8 +322,9 @@ static bool isFoldableArray(SILValue value, ASTContext &astContext) {
322322
return true;
323323
SILFunction *callee = cast<ApplyInst>(constructorInst)->getCalleeFunction();
324324
return !callee ||
325-
(!callee->hasSemanticsAttr("array.init.empty") &&
326-
!callee->hasSemanticsAttr("array.uninitialized_intrinsic"));
325+
(!callee->hasSemanticsAttr(semantics::ARRAY_INIT_EMPTY) &&
326+
!callee->hasSemanticsAttr(semantics::ARRAY_UNINITIALIZED_INTRINSIC) &&
327+
!callee->hasSemanticsAttr(semantics::ARRAY_FINALIZE_INTRINSIC));
327328
}
328329

329330
/// Return true iff the given value is a closure but is not a creation of a
@@ -997,6 +998,14 @@ static void substituteConstants(FoldState &foldState) {
997998
for (SILValue constantSILValue : foldState.getConstantSILValues()) {
998999
SymbolicValue constantSymbolicVal =
9991000
evaluator.lookupConstValue(constantSILValue).getValue();
1001+
// Make sure that the symbolic value tracked in the foldState is a constant.
1002+
// In the case of ArraySymbolicValue, the array storage could be a non-constant
1003+
// if some instruction in the array initialization sequence was not evaluated
1004+
// and skipped.
1005+
if (!constantSymbolicVal.containsOnlyConstants()) {
1006+
assert(constantSymbolicVal.getKind() != SymbolicValue::String && "encountered non-constant string symbolic value");
1007+
continue;
1008+
}
10001009

10011010
SILInstruction *definingInst = constantSILValue->getDefiningInstruction();
10021011
assert(definingInst);

‎stdlib/private/OSLog/OSLogTestHelper.swift‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,3 +113,55 @@ internal func _os_log_impl_test(
113113
start: UnsafePointer(bufferMemory),
114114
count: Int(bufferSize)))
115115
}
116+
117+
118+
119+
/// A function that pretends to be os_signpost(.animationBegin, ...). The purpose
120+
/// of this function is to test whether the OSLogOptimization pass works properly
121+
/// on the special case of animation begin signposts.
122+
@_transparent
123+
public func _osSignpostAnimationBeginTestHelper(
124+
_ format: AnimationFormatString.OSLogMessage,
125+
_ arguments: CVarArg...
126+
) {
127+
_animationBeginSignpostHelper(formatStringPointer: format.formatStringPointer,
128+
arguments: arguments)
129+
}
130+
131+
@usableFromInline
132+
internal func _animationBeginSignpostHelper(
133+
formatStringPointer: UnsafePointer<CChar>,
134+
arguments: [CVarArg]
135+
) {}
136+
137+
// A namespace for utilities specific to os_signpost animation tests.
138+
public enum AnimationFormatString {
139+
@inlinable
140+
@_optimize(none)
141+
@_semantics("constant_evaluable")
142+
internal static func constructOSLogInterpolation(
143+
_ formatString: String
144+
) -> OSLogInterpolation {
145+
var s = OSLogInterpolation(literalCapacity: 1, interpolationCount: 0)
146+
s.formatString += formatString
147+
s.formatString += " isAnimation=YES"
148+
return s
149+
}
150+
151+
@frozen
152+
public struct OSLogMessage : ExpressibleByStringLiteral {
153+
@usableFromInline
154+
var formatStringPointer: UnsafePointer<CChar>
155+
156+
@_transparent
157+
public init(stringLiteral value: String) {
158+
let message =
159+
OSLogTestHelper.OSLogMessage(
160+
stringInterpolation:
161+
constructOSLogInterpolation(
162+
value))
163+
let formatString = message.interpolation.formatString
164+
formatStringPointer = _getGlobalStringTablePointer(formatString)
165+
}
166+
}
167+
}

‎test/SILOptimizer/OSLogMandatoryOptTest.sil‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1055,3 +1055,59 @@ bb0:
10551055
// CHECK: bb0
10561056
// CHECK: alloc_stack $OSLogInterpolationDCEStub
10571057
}
1058+
1059+
// The following tests are for checking that constant folding doesn't
1060+
// cause a crash when evaulating the instructions of an animation signpost.
1061+
1062+
// Protocol stub for CVarArgStub
1063+
protocol CVarArgStub {}
1064+
1065+
// Ensure that Int conforms to our protocol stub
1066+
extension Int64 : CVarArgStub {}
1067+
1068+
// _finalizeUninitializedArray<A>(_:)
1069+
sil shared_external [serialized] [_semantics "array.finalize_intrinsic"] @$ss27_finalizeUninitializedArrayySayxGABnlF : $@convention(thin) <Element> (@owned Array<Element>) -> @owned Array<Element>
1070+
1071+
// Test that the OSLogOptimization does not fail while attempting to
1072+
// fold CVarArgStubs in animation signposts.
1073+
// CHECK-LABEL: @testFoldingOfCVarArgStubConstruction
1074+
sil [ossa] @testFoldingOfCVarArgStubConstruction : $@convention(thin) () -> () {
1075+
bb0:
1076+
// Construct an OSLogMessageStub instance.
1077+
%0 = string_literal utf8 "animation begins here %d"
1078+
%1 = integer_literal $Builtin.Word, 24
1079+
%2 = integer_literal $Builtin.Int1, -1
1080+
%3 = metatype $@thin String.Type
1081+
// function_ref String.init(_builtinStringLiteral:utf8CodeUnitCount:isASCII:)
1082+
%4 = function_ref @$sSS21_builtinStringLiteral17utf8CodeUnitCount7isASCIISSBp_BwBi1_tcfC : $@convention(method) (Builtin.RawPointer, Builtin.Word, Builtin.Int1, @thin String.Type) -> @owned String
1083+
%5 = apply %4(%0, %1, %2, %3) : $@convention(method) (Builtin.RawPointer, Builtin.Word, Builtin.Int1, @thin String.Type) -> @owned String // users: %60, %47
1084+
%6 = function_ref @oslogMessageInit : $@convention(thin) (@owned String) -> @owned OSLogMessageStub
1085+
%7 = apply %6(%5) : $@convention(thin) (@owned String) -> @owned OSLogMessageStub
1086+
1087+
// Begin chain of evaluated instructions
1088+
%8 = begin_borrow %7 : $OSLogMessageStub
1089+
1090+
// Construct CVarArgStub
1091+
%11 = integer_literal $Builtin.Word, 1
1092+
// function_ref _allocateUninitializedArray<A>(_:)
1093+
%12 = function_ref @$ss27_allocateUninitializedArrayySayxG_BptBwlF : $@convention(thin) <τ_0_0> (Builtin.Word) -> (@owned Array<τ_0_0>, Builtin.RawPointer)
1094+
%13 = apply %12<CVarArgStub>(%11) : $@convention(thin) <τ_0_0> (Builtin.Word) -> (@owned Array<τ_0_0>, Builtin.RawPointer)
1095+
(%14, %15) = destructure_tuple %13 : $(Array<CVarArgStub>, Builtin.RawPointer)
1096+
%16 = pointer_to_address %15 : $Builtin.RawPointer to [strict] $*CVarArgStub
1097+
%17 = integer_literal $Builtin.IntLiteral, 42
1098+
%18 = builtin "s_to_s_checked_trunc_IntLiteral_Int64"(%17 : $Builtin.IntLiteral) : $(Builtin.Int64, Builtin.Int1)
1099+
(%19, %20) = destructure_tuple %18 : $(Builtin.Int64, Builtin.Int1)
1100+
%21 = struct $Int64 (%19 : $Builtin.Int64)
1101+
%22 = init_existential_addr %16 : $*CVarArgStub, $Int64
1102+
store %21 to [trivial] %22 : $*Int64
1103+
// function_ref _finalizeUninitializedArray<A>(_:)
1104+
%23 = function_ref @$ss27_finalizeUninitializedArrayySayxGABnlF : $@convention(thin) <τ_0_0> (@owned Array<τ_0_0>) -> @owned Array<τ_0_0>
1105+
%24 = apply %23<CVarArgStub>(%14) : $@convention(thin) <τ_0_0> (@owned Array<τ_0_0>) -> @owned Array<τ_0_0>
1106+
destroy_value %24 : $Array<CVarArgStub>
1107+
1108+
// End chain of evaluated instructions
1109+
end_borrow %8 : $OSLogMessageStub
1110+
destroy_value %7 : $OSLogMessageStub
1111+
%25 = tuple ()
1112+
return %25 : $()
1113+
}

‎test/SILOptimizer/OSLogMandatoryOptTest.swift‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -623,3 +623,17 @@ func testWrappingWithinClosures(x: Int) {
623623
// CHECK-LABEL: end sil function '${{.*}}testWrappingWithinClosures1xySi_tFyyXEfU_
624624
}
625625
}
626+
627+
func testAnimationSignpost(cond: Bool, x: Int, y: Float) {
628+
_osSignpostAnimationBeginTestHelper("animation begins here %d", Int.min)
629+
_osSignpostAnimationBeginTestHelper("a message without arguments")
630+
_osSignpostAnimationBeginTestHelper("animation begins here %ld", x)
631+
_osSignpostAnimationBeginTestHelper("animation begins here %ld", cond ? x : y)
632+
_osSignpostAnimationBeginTestHelper("animation begins here %ld %f", x, y)
633+
// CHECK-LABEL: @${{.*}}testAnimationSignpost4cond1x1yySb_SiSftF
634+
// CHECK: string_literal utf8 "animation begins here %d isAnimation=YES"
635+
// CHECK: string_literal utf8 "a message without arguments isAnimation=YES"
636+
// CHECK: string_literal utf8 "animation begins here %ld isAnimation=YES"
637+
// CHECK: string_literal utf8 "animation begins here %ld isAnimation=YES"
638+
// CHECK: string_literal utf8 "animation begins here %ld %f isAnimation=YES"
639+
}

0 commit comments

Comments
 (0)