Remove linear map structs and use plain tuples instead. #63444
Conversation
|
Tagging @BradLarson @dan-zheng |
|
@swift-ci please test |
|
Nice approach! This seems to simplify things and avoids the struct complications. I wonder if there are still complications from the branching trace enums, since those are still |
|
@dan-zheng Yes. I'm afraid we can still cook a testcase that would trigger an assertion similar to #63207 but due to lack of imported branch trace enum type. Though, I believe it must be more complicated. |
| if (!origBB.isEntry()) { | ||
| CanType traceEnumType = getBranchingTraceEnumLoweredType(&origBB).getASTType(); | ||
| linearTupleTypes.emplace_back(traceEnumType, | ||
| astCtx.getIdentifier(traceEnumFieldName)); |
There was a problem hiding this comment.
... wait I don't think SIL tuples are allowed to have labels, as it's no longer a canonical type? Am I misremembering this?
There was a problem hiding this comment.
Well, it seems it does not cause any problems anywhere, the identifier is just stored in the type and that's it. I double checked – the generated SIL parses as well.
On the other hand – having named field for branch trace enum helps debugging as we're having internal contract with indices :)
There was a problem hiding this comment.
Ok. Something unrelated: would it be better if we made the branching trace enum be the last element? Might help with alignment in the tuple.
There was a problem hiding this comment.
Frankly speaking, I do not know :) I wanted to replicate the struct layout to minimize possible unknown fallback. This is certainly worth exploring if e.g. this might improve the codegen, etc.
| @@ -3433,6 +3433,36 @@ static void printSILDifferentiabilityWitnesses( | |||
| dw->print(Ctx.OS(), Ctx.printVerbose()); | |||
| } | |||
|
|
|||
| static void printSILLinearMapTypes(SILPrintContext &Ctx, | |||
There was a problem hiding this comment.
Is this only for printing branching trace enums now?
Yeah, right. But maybe as a next step? :) |
|
Sure, this is already strictly an improvement. |
|
@swift-ci please test |
|
@rxwei I'm feeling like opening a can of worms as many things require original AST types, while we'd need to operate mostly on lowered SIL ones :) I thought that I fixed all such places, but apparently not. The last failure was caused by assertion inside generic cloner as |
|
@swift-ci please test |
|
MacOS test is broken due to broken main after #63423 |
|
@swift-ci please test |
|
@swift-ci please test macos platform |
1 similar comment
|
@swift-ci please test macos platform |
The changes are intentionally were made close to the original implementation w/o possible simplifications to ease the review
Fixes #63207, supersedes #63379 (and fixes #63234)