[PackageCMO] Use InstructionVisitor to check inst serializability.

[elsh]

Currently, InstructionVisitor is only used during the serialization pass,
that comes after a pass that checks if instructions are serializable. This
causes inconsistencies as some types bypass the first pass but are processed
in the second, leading to assert fails.

This PR uses InstructionVisitor in the first pass to be exhaustive and to
be consistent with the coverage in the second pass. The downside is the visitor
is SILCloner, which introduces overhead of cloning and discarding insts per pass
-- a potential area for future refactoring.

Resolves rdar://130788545.

[eeckstein]

On the one hand, this is the right thing to do. On the other hand it means that we are (potentially) cloning the whole SIL in this pass. It's really unfortunate that the visit logic is so much baked into the Cloner that it's not really possible to separate them.

[elsh]

@swift-ci smoke test

[aschwaighofer]

I don't think this dense map is needed.

Instead the InstructionVisitor instance should be discarded after/scoped for each SILInstruction that is visited and the isInstSerializable queried.

i.e something like:

for (auto inst : block) {

   InstructionVisitor visitor(...);
   visitor.visit(inst);
   if (!visitor.isInstSerializableSet())
     return false; // can't serialize inst
   ...
}

[elsh]

The visitor was created per function instead of per inst to match how it was created in the serialization pass; https://github.com/swiftlang/swift/blame/3d40a8c9f2660294234f08ac47680544f547202d/lib/SILOptimizer/IPO/CrossModuleOptimization.cpp#L733 cc @slavapestov

[aschwaighofer]

I don't see a need of this here? There is no state that needs to be carried across one iteration of the loop but instead the bigger scope across multiple iterations forces the use of the map (seems to force, see next comment).

... thinking about this some more. I think you can get rid of the map and leave the scope of the visitor (since we exist early once the isSerializable state changes) as is:

The purpose of this loop is to find the first instruction that has a type that can't be serialized. You start of with isSerializable = true. You are iterating over instructions collecting their types, checking if any of the type you encounter is not serializable. Once you encounter the first one that is not serializable (isSerializable = false) you exist the loop and don't "look at the visitor again". No need for the map and you can leave the Visitor in the scope it currently sits in because we won't look at it again after we found an instruction that does not serialize.

[elsh]

I agree per-instruction visit won't require a map, but I think the reason for creating visitor per function during the serialization pass was so that unresolved local archetypes can be handled at https://github.com/swiftlang/swift/blame/3d40a8c9f2660294234f08ac47680544f547202d/lib/SILOptimizer/IPO/CrossModuleOptimization.cpp#L742 in recursive calls, and the new pass that checks serializability should also follow the same path. Let me know if that's a wrong assumption.

[elsh]

Update: removed the map

[elsh]

@swift-ci smoke test

[elsh]

@swift-ci smoke test

[elsh]

@swift-ci smoke test

[elsh]

@swift-ci smoke test

[elsh]

@swift-ci smoke test

[elsh]

@swift-ci smoke test

[elsh]

@swift-ci smoke test

[elsh]

@swift-ci smoke test

[aschwaighofer]

lgtm