SIL: Update formal source type in SimplifyRefCasts when possible #78996
Conversation
aschwaighofer
requested review from
eeckstein,
adrian-prantl and
jckarter
as code owners
|
@swift-ci test |
| // formal source type lowered equaled the operand SIL type before | ||
| // transformation). | ||
| let formalSrcType = ccb.formalSrcType | ||
| if formalSrcType.loweredType(in: parentFunction, maximallyAbstracted: false) == oldOperandType { |
There was a problem hiding this comment.
Why this weird check?
It seems we can't always use the operand`s lowered SIL type (SIL type lowering has lost some information?! unfortunate ...) but need to carry the formal type.
So this check here checks that the when we lower the formal type to a SIL type we get the same type ie. we are not one of the cases where during the process of SIL lowering information was lost.
There was a problem hiding this comment.
Except, that as I write this I realize the check should be formalSrcType == oldOperandType.astType
aschwaighofer
force-pushed
the
checked_cast_br_nightmares
branch
from
da2730a to
44f03b9
Compare
|
@swift-ci test |
| @@ -86,6 +88,15 @@ private extension UnaryInstruction { | |||
| operand.set(to: replacement, context) | |||
| } | |||
|
|
|||
| if let ccb = self as? CheckedCastBranchInst { | |||
| // Make sure that loosing the formal source type is harmless (i.e the | |||
There was a problem hiding this comment.
I don't understand this comment. I think what you want to check is the following:
if the lowered source operand type is the formal type before the transformation we can assume that the type of the new operand is also a formal type.
Is this assumption always true?
Wouldn't it be better to just check if the new operand type isLegalFormalType()?
There was a problem hiding this comment.
Forgot to update the comment after updating the code. Have you seen my comment on this PR above?
Yes you are right: The comment should say: if the lowered source operand type is the formal type before the transformation we can assume that no information was lost during type lowering.
Does isLegalFormalType check this condition?
It is not clear to me that this is why checked _cast_addr_br needs the formal source type:
#67583
There was a problem hiding this comment.
It is not clear to me that this is why checked _cast_addr_br needs the formal source type:
It's used in IRGen (visitCheckedCastAddrBranchInst). I assume the runtime type/cast machinery needs formal types rather than lowered types.
Does isLegalFormalType check this condition?
I think if the new SILType is a formal type (which isLegalFormalType should check) it's fine to use it.
There was a problem hiding this comment.
Okay, let's use that.
| @@ -11032,6 +11032,10 @@ class CheckedCastBranchInst final | |||
| SILType getSourceLoweredType() const { return getOperand()->getType(); } | |||
| CanType getSourceFormalType() const { return SrcFormalTy; } | |||
|
|
|||
| void updateSourceFormalTypeFromOperandLoweredType() { | |||
There was a problem hiding this comment.
Why not setSourceFormalType(CanType newFormalType)?
There was a problem hiding this comment.
Add the ability to set any unrelated formal type?
We use the formal source type do decide whether a checked_cast_br is known to succeed/fail. If we don't update it we loose that optimization That is: ``` checked_cast_br AnyObject in %2 : X to X, bb1, bb2 ``` Will not be simplified even though the operand and the destintation type matches.
aschwaighofer
force-pushed
the
checked_cast_br_nightmares
branch
from
44f03b9 to
0fe3cf0
Compare
|
@swift-ci test |
We use the formal source type do decide whether a checked_cast_br is known to succeed/fail. If we don't update it we loose that optimization
That is:
Will not be simplified even though the operand and the destintation type matches.