Fixed a bug in const folding, where an inst that's not of type SingleValueInstruction gets added to the worklist #18290
Conversation
SingleValueInstruction gets added to the worklist, causing cast assert at https://github.com/apple/swift/blame/master/lib/SILOptimizer/Utils/ConstantFolding.cpp#L1585. One such example inst is the following (in the tensorflow branch), which produces a SILValue of type MultipleValueInstructionResult, so ValueBase::getDefiningInstruction() still returns a valid inst for it, even though that graph_op inst is not a SingleValueInstruction. ``` %94 = graph_op "Fill,i,i"(%73 : $TensorHandle<Int32>, %85 : $TensorHandle<Float>) {T: $Float, index_type: $Int32, __device: "/device:CPU:0"} : $TensorHandle<Float> ``` The same fix has been merged into the tensorflow branch: swiftlang#18272
|
@swift-ci please smoke test |
|
Hi @eeckstein, I don't know how to write a simple test case for that (due to my lack of familiarity with the const folding code base) -- which multi value inst to use, and how to get it into the worklist. In the tensorflow branch, it was triggered by a fairly involved program. From code reading it seems relatively straightforward that this fix is needed, and is safe. A test on it would be a "negative test" -- was crashing before, and no longer crashes. I assume it's not worth aiming for complete coverage for such negative tests. What do you think, and any suggestions? |
|
In general there should be regression tests for all bugs which are fixed. For this fix, you can write a SIL test with a multi-result instructions (see MULTIPLE_VALUE_INST_RESULT in SILNodes.def to pick one). For examples see test/SILOptimizer/constant_propagation.sil. You can add the test in this file. |
|
Thank you for the pointers! I looked into the existing inst types (e.g. destructure_tuple, destructure_struct), but couldn't find one that yields a simple test case -- that could explain why this bug has (presumably) not been hit by anyone so far. We have a test case in the tensorflow branch: #18412, and plan to upstream the branch code to master branch gradually. WDYT? |
|
Hi @eeckstein, is this PR go to merge? Thanks. |
sounds good. |
This causes cast assert at https://github.com/apple/swift/blame/master/lib/SILOptimizer/Utils/ConstantFolding.cpp#L1585.
One such example inst is the following (in the tensorflow branch), which produces a SILValue of type
MultipleValueInstructionResult, so ValueBase::getDefiningInstruction() still
returns a valid inst for it, even though that graph_op inst is not a SingleValueInstruction.
The same fix has been merged into the tensorflow branch: #18272