Fix Mem2Reg for store [assign] #35805
Conversation
meg-gupta
changed the title
|
@swift-ci test |
| @@ -459,6 +459,7 @@ StackAllocationPromoter::promoteAllocationInBlock(SILBasicBlock *BB) { | |||
| // RunningVal is the current value in the stack location. | |||
| // We don't know the value of the alloca until we find the first store. | |||
| SILValue RunningVal = SILValue(); | |||
| SILValue LastRunningVal = SILValue(); | |||
There was a problem hiding this comment.
You should add a comment here explaining what LastRunningVal is and why it is different than RunningVal.
There was a problem hiding this comment.
You could also change this to be called PreviousRunningVal because "last" might mean "the last in a list" whereas previous always means "the one before this one".
There was a problem hiding this comment.
The information that I would want here is something with the following information (feel free to wordsmith).
LastRunningVal is the value that we saw stored in the stack
location /immediately before/ we updated the RunningVal at the
last store. The reason why we track this information on top of
the current running value is that if the last store was an assign
and turns out to be dead, we need to insert a destroy_value on
that value.
| assert(RunningVal); | ||
| // We have to create a destroy_value of the RunningVal when we see a store | ||
| // [assign]. | ||
| // A RunningVal will be be available if : |
There was a problem hiding this comment.
Can you clean up this comment. My thought:
// We have to create a destroy_value of the RunningVal when we see a store
// [assign].
//
// A RunningVal will be available if:
//
// 1. store [assign] was in an entry block.
// 2. store [assign] is not the first store in a non-entry block
| @@ -459,6 +459,7 @@ StackAllocationPromoter::promoteAllocationInBlock(SILBasicBlock *BB) { | |||
| // RunningVal is the current value in the stack location. | |||
| // We don't know the value of the alloca until we find the first store. | |||
| SILValue RunningVal = SILValue(); | |||
| SILValue LastRunningVal = SILValue(); | |||
There was a problem hiding this comment.
You could also change this to be called PreviousRunningVal because "last" might mean "the last in a list" whereas previous always means "the one before this one".
This PR simplifies the handling of store [assign] in Mem2Reg by always converting it to a store [init]. Also fixes an edge case where store [assign] in a non entry block was not the first store of an alloc_stack with uses in multiple blocks.
|
@gottesmm thanks for suggesting transforming store [assign] -> store [init]. store [assign] handling is much simpler now. |
|
@swift-ci test |
|
@swift-ci test windows platform |
This PR simplifies the handling of store [assign] in Mem2Reg by always converting it to a store [init].
Also fixes an edge case where store [assign] in a non entry block was not the first store of an alloc_stack with uses in multiple blocks.