Ensure we are using mapped SIL type for switch_enum case and not the original lowered one #73385

asl · 2024-05-01T22:12:05Z

asl · 2024-05-01T22:12:13Z

asl · 2024-05-01T22:12:17Z

slavapestov · 2024-05-01T22:29:31Z

test/IRGen/loadable_by_address_issue73018.swift

+// switch_enum arguments during rewriting
+
+import Foundation; import _Differentiation
+struct O: Differentiable {var a: B.G; var b: Array<SIMD2<Float>>; var c: B.M; var d: B.M}


Is there a smaller reduction that doesn’t use Differentiable?

I do not have one sadly. Differentiation tends to create switch_enum's with large payloads having tuples with closures inside. So, this exposes all kinds of corner cases in LoadableByAddress.

The type in question here is:

$(@callee_guaranteed (@guaranteed C<Float>) -> @owned Array<Float>.DifferentiableView, @callee_guaranteed (@guaranteed C<Float>) -> @owned Array<Float>.DifferentiableView, @callee_guaranteed (@guaranteed C<Float>) -> @owned Array<Float>.DifferentiableView, @callee_guaranteed (@guaranteed C<SIMD2<Float>>) -> @owned Array<SIMD2<Float>>.DifferentiableView, @callee_guaranteed (@guaranteed C<Float>) -> @owned Array<Float>.DifferentiableView, @callee_guaranteed (@guaranteed C<Float>) -> @owned Array<Float>.DifferentiableView, @callee_guaranteed (@guaranteed B.M.TangentVector) -> @owned B.M.TangentVector, @callee_guaranteed (@guaranteed B.M.TangentVector) -> @owned B.M.TangentVector))

that is mapped to:

$(@callee_guaranteed (@guaranteed C<Float>) -> @owned Array<Float>.DifferentiableView, @callee_guaranteed (@guaranteed C<Float>) -> @owned Array<Float>.DifferentiableView, @callee_guaranteed (@guaranteed C<Float>) -> @owned Array<Float>.DifferentiableView, @callee_guaranteed (@guaranteed C<SIMD2<Float>>) -> @owned Array<SIMD2<Float>>.DifferentiableView, @callee_guaranteed (@guaranteed C<Float>) -> @owned Array<Float>.DifferentiableView, @callee_guaranteed (@guaranteed C<Float>) -> @owned Array<Float>.DifferentiableView, @callee_guaranteed (@in_guaranteed B.M.TangentVector) -> @out B.M.TangentVector, @callee_guaranteed (@in_guaranteed B.M.TangentVector) -> @out B.M.TangentVector)

And everything seems to depend on the order of previous transformations. I cannot remove any single line in the reproducer to still trigger an assertion, looks like we need to hit some thresholds as in:

bool isLargeLoadableType(SILType ty) { if (ty.isAddress() || ty.isClassOrClassMetatype()) { return false; } auto canType = ty.getASTType(); if (canType->hasTypeParameter()) { assert(genEnv && "Expected a GenericEnv"); canType = genEnv->mapTypeIntoContext(canType)->getCanonicalType(); } if (canType.getAnyGeneric() || isa<TupleType>(canType)) { assert(ty.isObject() && "Expected only two categories: address and object"); assert(!canType->hasTypeParameter()); const TypeInfo &TI = irgenModule->getTypeInfoForLowered(canType); auto &nativeSchemaOrigParam = TI.nativeParameterValueSchema(*irgenModule); if (nativeSchemaOrigParam.size() > 15) return true; auto explosionSchema = TI.getSchema(); if (explosionSchema.size() > 15) return true; } return false; }

The thresholds in that function are just a property of the stored property layout of the type and it should be possible to hit them by declaring, eg, a struct with a tuple containing 100 elements.

asl · 2024-05-01T23:08:40Z

@swift-ci please test

aschwaighofer

Thank you for looking into that!

aschwaighofer · 2024-05-02T17:10:54Z

lib/IRGen/LoadableByAddress.cpp

@@ -4077,9 +4087,15 @@ class RewriteUser : SILInstructionVisitor<RewriteUser> {
             "caseBB has a payload argument");

      SILBuilder caseBuilder = assignment.getBuilder(caseBB->begin());
+      SILType eltType =


I would prefer to keep the type lowering code out of this phase.

It should be possible to read lowered (mapped) types out of the "input" SIL to this phase.

In this case, the result type is the switch_enum's successor basic block argument type.

This should work, I think:

diff --git a/lib/IRGen/LoadableByAddress.cpp b/lib/IRGen/LoadableByAddress.cpp index d76c9fc3310..2502d55bc70 100644 --- a/lib/IRGen/LoadableByAddress.cpp +++ b/lib/IRGen/LoadableByAddress.cpp @@ -4078,7 +4078,8 @@ protected: SILBuilder caseBuilder = assignment.getBuilder(caseBB->begin()); auto *caseAddr = - caseBuilder.createUncheckedTakeEnumDataAddr(loc, opdAddr, caseDecl); + caseBuilder.createUncheckedTakeEnumDataAddr(loc, opdAddr, caseDecl, + caseArg->getType().getAddressType()); if (assignment.isLargeLoadableType(caseArg->getType())) { assignment.mapValueToAddress(caseArg, caseAddr);

Oh, indeed here!

asl · 2024-05-02T17:34:11Z

@swift-ci please test

slavapestov

The code change looks good but it still needs a better test case.

aschwaighofer · 2024-05-03T16:40:18Z

A reproducer could look something like:

import Builtin
import Swift
struct X {
  var x1 : Int
  var x2 : Int
  var x3 : Int
  var x4: Int
  var x5: Int
  var x6: Int
  var x7: Int
  var x8: Int
  var x9: Int
  var x10: Int
  var x11: Int
  var x12: Int
  var x13: Int
  var x14: Int
  var x15: Int
  var x16: Int
}
enum large_enum {
  case bb0(( () -> X, () -> X, () -> X, () -> X, () -> X, () -> X, () -> X, () -> X, () -> X, () -> X, () -> X, () -> X, () -> X, () -> X, () -> X, () -> X))
}

sil @test1 : $@convention(thin) (@guaranteed large_enum) -> () {
bb0(%arg : $large_enum):
  %loc = alloc_stack $(@callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
                       @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
                       @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
                       @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
                       @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
                       @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
                       @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
                       @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X)
  switch_enum %arg : $large_enum, case #large_enum.bb0!enumelt: bb1

bb1(%p : $((@callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
            @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
            @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
            @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
            @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
            @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
            @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
            @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X))):
  store %p to %loc : $*(@callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
                        @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
                        @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
                        @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
                        @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
                        @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
                        @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
                        @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X)

  dealloc_stack %loc : $*(@callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
                          @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
                          @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
                          @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
                          @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
                          @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
                          @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X,
                          @callee_guaranteed () -> @out X, @callee_guaranteed () -> @out X)
  %t = tuple ()
  return %t : $()
}

Unfortunately, if you run swift-macosx-arm64/bin/swiftc -Xllvm -sil-print-after=loadable-address r.sil 2>&1 | less on this, a bunch of passes and the rest of LoadableByAddress runs prior to runPeepholesAndReg2Mem.
Maybe, this can be massage somehow to get to the crash by disabling the SIL passes that run prior (-Xllvm -sil-disable-pass=) and adding an option to disable what LoadableByAddress does prior to running runPeepholesAndReg2Mem.

asl · 2024-05-03T20:38:56Z

After few hours of experiments, I think I ended with smaller reproducer. Note that it needs to be used with disabled SIL verifier (-Xllvm -verify-continue-on-failure) due to exactly same issue as fixed in the PR (results are lowered as @out, but we'd need them @owned, but there is no way to specify this on Swift level):

import Builtin
import Swift

typealias X = Int
typealias LargeX = (() -> X, () -> X, () -> X, () -> X, () -> X, () -> X, () -> X, () -> X, () -> X)

enum enum1 {
case bb0(LargeX)
}

enum enum2 {
case bb0(LargeX)
}

enum large_enum {
case bb1((enum1, X))
case bb2((enum2, X))
}

sil @test1 : $@convention(thin) (@guaranteed large_enum) -> () {
bb0(%arg : $large_enum):
  %loc = alloc_stack $(@callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,
                       @callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,
                       @callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,
                       @callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,                       
                       @callee_guaranteed () -> @owned X)
  switch_enum %arg : $large_enum, case #large_enum.bb1!enumelt: bb1, case #large_enum.bb2!enumelt: bb2

bb1(%e1 : $(enum1, X)):
  %e11 = tuple_extract %e1 : $(enum1, X), 0
  switch_enum %e11 : $enum1, case #enum1.bb0!enumelt: bb11

bb11(%p1 : $((@callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,
              @callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,
              @callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,              
              @callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,
              @callee_guaranteed () -> @owned X))):
  br bb3(%p1 : $((@callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,
                  @callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,
                  @callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,
                  @callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,                  
                  @callee_guaranteed () -> @owned X)))                      

bb2(%e2 : $(enum2, X)):
  %e22 = tuple_extract %e2 : $(enum2, X), 0
  switch_enum %e22 : $enum2, case #enum2.bb0!enumelt: bb22

bb22(%p2 : $((@callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,
              @callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,
              @callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,
              @callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,
              @callee_guaranteed () -> @owned X))):
  br bb3(%p2 : $((@callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,
                  @callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,  
                  @callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,
                  @callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,            
                  @callee_guaranteed () -> @owned X)))                      

bb3(%p3 : $((@callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,
             @callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,
             @callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,
             @callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,              
             @callee_guaranteed () -> @owned X))):
  store %p3 to %loc : $*(@callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,
                         @callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,
                         @callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,
                         @callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,              
                         @callee_guaranteed () -> @owned X)
            
  dealloc_stack %loc : $*(@callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,
                          @callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,
                          @callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,
                          @callee_guaranteed () -> @owned X, @callee_guaranteed () -> @owned X,              
                          @callee_guaranteed () -> @owned X)
  %t = tuple ()
  return %t : $()
}

Are we ok with this? @slavapestov @aschwaighofer

original lowered one. Fixes #73018

asl · 2024-05-08T21:57:03Z

@swift-ci please test

aschwaighofer

I am fine with the test case

…original lowered one (swiftlang#73385) Fixes swiftlang#73018 (cherry picked from commit e7e2ad1) (cherry picked from commit 422fe83)

…original lowered one (swiftlang#73385) Fixes swiftlang#73018 (cherry picked from commit e7e2ad1)

…original lowered one (#73385) Fixes #73018 (cherry picked from commit e7e2ad1)

asl requested a review from aschwaighofer May 1, 2024 22:12

slavapestov reviewed May 1, 2024

View reviewed changes

asl force-pushed the 73018-fix branch from 5551fa2 to db164d7 Compare May 1, 2024 23:08

aschwaighofer requested changes May 2, 2024

View reviewed changes

asl force-pushed the 73018-fix branch from db164d7 to 5fbca17 Compare May 2, 2024 17:33

asl requested a review from aschwaighofer May 2, 2024 17:34

aschwaighofer approved these changes May 3, 2024

View reviewed changes

slavapestov reviewed May 3, 2024

View reviewed changes

asl requested review from slavapestov and aschwaighofer May 7, 2024 21:02

Ensure we are using mapped SIL type for switch_enum case and not the

0797b05

original lowered one. Fixes #73018

asl force-pushed the 73018-fix branch from 5fbca17 to 0797b05 Compare May 8, 2024 21:56

aschwaighofer approved these changes May 9, 2024

View reviewed changes

asl merged commit e7e2ad1 into main May 9, 2024

asl deleted the 73018-fix branch May 9, 2024 20:41

clackary mentioned this pull request Oct 18, 2024

[6.0] Ensure we are using mapped SIL type for switch_enum case and not the original lowered one #77107

Merged

clackary pushed a commit to clackary/swift that referenced this pull request Nov 5, 2024

Ensure we are using mapped SIL type for switch_enum case and not the …

7c5b4ea

…original lowered one (swiftlang#73385) Fixes swiftlang#73018 (cherry picked from commit e7e2ad1)

DougGregor pushed a commit that referenced this pull request Dec 3, 2024

Ensure we are using mapped SIL type for switch_enum case and not the …

422fe83

…original lowered one (#73385) Fixes #73018 (cherry picked from commit e7e2ad1)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Ensure we are using mapped SIL type for switch_enum case and not the original lowered one #73385

Ensure we are using mapped SIL type for switch_enum case and not the original lowered one #73385

Uh oh!

asl commented May 1, 2024

Uh oh!

asl commented May 1, 2024

Uh oh!

asl commented May 1, 2024

Uh oh!

slavapestov May 1, 2024

Uh oh!

asl May 1, 2024

Uh oh!

slavapestov May 2, 2024

Uh oh!

asl commented May 1, 2024

Uh oh!

aschwaighofer left a comment

Uh oh!

aschwaighofer May 2, 2024

Uh oh!

asl May 2, 2024

Uh oh!

asl commented May 2, 2024

Uh oh!

slavapestov left a comment

Uh oh!

aschwaighofer commented May 3, 2024

Uh oh!

asl commented May 3, 2024 •

edited

Loading

Uh oh!

asl commented May 8, 2024

Uh oh!

aschwaighofer left a comment

Uh oh!

Uh oh!

Ensure we are using mapped SIL type for switch_enum case and not the original lowered one #73385

Ensure we are using mapped SIL type for switch_enum case and not the original lowered one #73385

Uh oh!

Conversation

asl commented May 1, 2024

Uh oh!

asl commented May 1, 2024

Uh oh!

asl commented May 1, 2024

Uh oh!

slavapestov May 1, 2024

Choose a reason for hiding this comment

Uh oh!

asl May 1, 2024

Choose a reason for hiding this comment

Uh oh!

slavapestov May 2, 2024

Choose a reason for hiding this comment

Uh oh!

asl commented May 1, 2024

Uh oh!

aschwaighofer left a comment

Choose a reason for hiding this comment

Uh oh!

aschwaighofer May 2, 2024

Choose a reason for hiding this comment

Uh oh!

asl May 2, 2024

Choose a reason for hiding this comment

Uh oh!

asl commented May 2, 2024

Uh oh!

slavapestov left a comment

Choose a reason for hiding this comment

Uh oh!

aschwaighofer commented May 3, 2024

Uh oh!

asl commented May 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asl commented May 8, 2024

Uh oh!

aschwaighofer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

asl commented May 3, 2024 •

edited

Loading