[SILOptimizer] shouldExpand can't expand address only types

[shajrawi]

fix a bug wherein EnableExpandAll overrode that.

rdar://problem/33767770

[shajrawi]

@swift-ci Please smoke test and merge

[shajrawi]

@swift-ci Please smoke test and merge

[shajrawi]

@swift-ci Please clean smoke test OS X

[slavapestov]

Test case?

[shajrawi]

@slavapestov The current uses of shouldExpand all over the optimizer also check for resilience so all test cases will not trigger the buggy code path. I changed this check as a defensive measure while auditing the optimizer for resilience - in case someone calls this function without checking resilience in the future.

[slavapestov]

In that case can you turn it into an assert instead? If we don't want people calling the function without checking resilience, let's assert instead of failing silently.

[shajrawi]

I am not sure we don't want people calling it like that / prevent it: the function just tells the user if they should explode a type 'T' or not. I see no reason why calling it with an address only type should crash instead of saying: no, you can't explode this type.

[shajrawi]

(it is, after all, just a utility function. Its name doesn't indicate that the compiler should panic just because someone called it as part of their heuristics for doing an optimization 'x' or not)

[shajrawi]

@swift-ci Please clean smoke test Linux

[slavapestov]

@shajrawi Either all callers should check for resilience and not call the function with resilient types, or the function should do the check and return false. Having both just creates confusion IMO.

[shajrawi]

I think the function should do the check itself regarding your point above. without crashing for resilient types / guarding the user of course. especially if a user calls this heuristic on all types as an analysis before going forward with an optimization or not. the problem with that is we have cases like the one below:

  bool expand = shouldExpand(M, SrcType.getObjectType());
  using TypeExpansionKind = Lowering::TypeLowering::TypeExpansionKind;
  auto expansionKind = expand ? TypeExpansionKind::MostDerivedDescendents
                              : TypeExpansionKind::None;

  SILBuilderWithScope Builder(CA);

  // %new = load %0 : $*T
  LoadInst *New = Builder.createLoad(CA->getLoc(), Source,
                                     LoadOwnershipQualifier::Unqualified);

Wherein, being guarded by a pervious check, we know we can do a load even if the function returns false for code size reasons...