Late cast expansion: remove the null check if possible

[EgorBo]

If assertionprop sees that input object is already having a nonnull assertion we can give fgLateCastExpand a hint that we don't need a nullcheck, example:

// PGO warmupfor(inti=0;i<200;i++){Test(newProgram());Thread.Sleep(10);}[MethodImpl(MethodImplOptions.NoInlining)]voidTest(object?o){if(o!=null){if(oisProgram){Console.WriteLine();}}}

Diff: https://www.diffchecker.com/JkdxRigp/

Initially, I wanted to just introduce new _NONNULL cast helpers, but this change seems to be a lot smaller. In case if we run out of bits in gtMoreCallFlags we can remove it in favor of _NONNULL helpers.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

---

If assertionprop sees that input object is already having a nonnull assertion we can give fgLateCastExpand a hint that we don't need a nullcheck, example:

// PGO warmupfor(inti=0;i<200;i++){Test(newProgram());Thread.Sleep(10);}[MethodImpl(MethodImplOptions.NoInlining)]voidTest(object?o){if(o!=null){if(oisProgram){Console.WriteLine();}}}

Diff: https://www.diffchecker.com/JkdxRigp/

Initially, I wanted to just introduce new _NONNULL cast helpers, but this change seems to be a lot smaller. In case if we run out of bits in gtMoreCallFlags in can be removed in favor of _NONNULL helpers.

Author:	EgorBo
Assignees:	EgorBo
Labels:	area-CodeGen-coreclr
Milestone:	-

[EgorBo]

@dotnet/jit-contrib @jakobbotsch PTAL

gtMoreFlags has 5 spare bits and this PR uses 1 of them. An alternative option is to introduce new fake *_NONNULL cast helpers - but that is a bit more changes + JIT-EE update (and R2R bump?)

Overall it looks like if we run out of spare bits we can move some rare things to side dictionaries or make some of them context-dependent. E.g. 3 GDV flags are only needed for virtual calls and only during early phases. GTF_CALL_M_EXP_RUNTIME_LOOKUP is only used in Debug and GTF_CALL_M_CAST_CAN_BE_EXPANDED will be hopefully removed next week.

Diffs aren't too big, but they will be a lot bigger once I move non-profiled (regular) casts to the late phase.

Build failures are known.

[jakobbotsch]

I definitely think having separate helpers would be better, especially since it would be slightly better for unprofiled casts too (assuming we would have cast variants that skip the null check inside them).

I'm ok with using a flag, but note that having flags that alter the semantics of nodes is IMO bad IR design. It's super easy to forget to check for. For example, shouldn't this PR check for the new flag inside GenTreeCall::Equals?

[EgorBo]

I don't have a strong preference here, I'll see what it will take to use the helpers, but I am guessing it's a copy-paste of all 8 helpers everywhere + R2R version bump + JIT-EE bump.

assuming we would have cast variants that skip the null check inside them

I thought about it too, but sounds like adding extra several kb to CastHelpers.cs to skip a nullcheck so unlikely worth the effort.

the semantics of nodes is IMO bad IR design

I agree generally, but looks like some of the existing flags already do similar things? e.g. tail call

[jakobbotsch]

I agree generally, but looks like some of the existing flags already do similar things? e.g. tail call

Yes, we have a number of flags that alter semantics, GTF_UNSIGNED being one of the big ones. I think GenTreeCall::Equals is also missing the checks for tail call flags (IIRC we talked about this one before).

I'm ok with merging this as is (with the GenTreeCall::Equals fix). If we think about a new representation it doesn't necessarily have to be via a different helper num if we do not think that's beneficial in the unprofiled case (in fact, I think it shouldn't be then). For example, we could have GT_ISCLASS, GT_ISCLASS_NONULL, GT_TLSLOOKUP, GT_RUNTIMELOOKUP when optimizing and create the GT_CALL only later.

[EgorBo]

@jakobbotsch I've fixed the GenTreeCall::Equals case + I addressed your other feedback regarding removing the debug only flag and gtGetEffectiveValue

[EgorBo]

Failure is #97049

[EgorBo]

If assertionprop sees that input object is already having a nonnull assertion we can give fgLateCastExpand a hint that we don't need a nullcheck, example:

// PGO warmupfor(inti=0;i<200;i++){Test(newProgram());Thread.Sleep(10);}[MethodImpl(MethodImplOptions.NoInlining)]voidTest(object?o){if(o!=null){if(oisProgram){Console.WriteLine();}}}

Diff: https://www.diffchecker.com/JkdxRigp/

Initially, I wanted to just introduce new _NONNULL cast helpers, but this change seems to be a lot smaller. In case if we run out of bits in gtMoreCallFlags we can remove it in favor of _NONNULL helpers.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

---

If assertionprop sees that input object is already having a nonnull assertion we can give fgLateCastExpand a hint that we don't need a nullcheck, example:

// PGO warmupfor(inti=0;i<200;i++){Test(newProgram());Thread.Sleep(10);}[MethodImpl(MethodImplOptions.NoInlining)]voidTest(object?o){if(o!=null){if(oisProgram){Console.WriteLine();}}}

Diff: https://www.diffchecker.com/JkdxRigp/

Initially, I wanted to just introduce new _NONNULL cast helpers, but this change seems to be a lot smaller. In case if we run out of bits in gtMoreCallFlags in can be removed in favor of _NONNULL helpers.

Author:	EgorBo
Assignees:	EgorBo
Labels:	area-CodeGen-coreclr
Milestone:	-

[EgorBo]

@dotnet/jit-contrib @jakobbotsch PTAL

gtMoreFlags has 5 spare bits and this PR uses 1 of them. An alternative option is to introduce new fake *_NONNULL cast helpers - but that is a bit more changes + JIT-EE update (and R2R bump?)

Overall it looks like if we run out of spare bits we can move some rare things to side dictionaries or make some of them context-dependent. E.g. 3 GDV flags are only needed for virtual calls and only during early phases. GTF_CALL_M_EXP_RUNTIME_LOOKUP is only used in Debug and GTF_CALL_M_CAST_CAN_BE_EXPANDED will be hopefully removed next week.

Diffs aren't too big, but they will be a lot bigger once I move non-profiled (regular) casts to the late phase.

Build failures are known.

[jakobbotsch]

I definitely think having separate helpers would be better, especially since it would be slightly better for unprofiled casts too (assuming we would have cast variants that skip the null check inside them).

I'm ok with using a flag, but note that having flags that alter the semantics of nodes is IMO bad IR design. It's super easy to forget to check for. For example, shouldn't this PR check for the new flag inside GenTreeCall::Equals?

[EgorBo]

I don't have a strong preference here, I'll see what it will take to use the helpers, but I am guessing it's a copy-paste of all 8 helpers everywhere + R2R version bump + JIT-EE bump.

assuming we would have cast variants that skip the null check inside them

I thought about it too, but sounds like adding extra several kb to CastHelpers.cs to skip a nullcheck so unlikely worth the effort.

the semantics of nodes is IMO bad IR design

I agree generally, but looks like some of the existing flags already do similar things? e.g. tail call

[jakobbotsch]

I agree generally, but looks like some of the existing flags already do similar things? e.g. tail call

Yes, we have a number of flags that alter semantics, GTF_UNSIGNED being one of the big ones. I think GenTreeCall::Equals is also missing the checks for tail call flags (IIRC we talked about this one before).

I'm ok with merging this as is (with the GenTreeCall::Equals fix). If we think about a new representation it doesn't necessarily have to be via a different helper num if we do not think that's beneficial in the unprofiled case (in fact, I think it shouldn't be then). For example, we could have GT_ISCLASS, GT_ISCLASS_NONULL, GT_TLSLOOKUP, GT_RUNTIMELOOKUP when optimizing and create the GT_CALL only later.

[EgorBo]

@jakobbotsch I've fixed the GenTreeCall::Equals case + I addressed your other feedback regarding removing the debug only flag and gtGetEffectiveValue

[EgorBo]

Failure is #97049

[EgorBo]

If assertionprop sees that input object is already having a nonnull assertion we can give fgLateCastExpand a hint that we don't need a nullcheck, example:

// PGO warmupfor(inti=0;i<200;i++){Test(newProgram());Thread.Sleep(10);}[MethodImpl(MethodImplOptions.NoInlining)]voidTest(object?o){if(o!=null){if(oisProgram){Console.WriteLine();}}}

Diff: https://www.diffchecker.com/JkdxRigp/

Initially, I wanted to just introduce new _NONNULL cast helpers, but this change seems to be a lot smaller. In case if we run out of bits in gtMoreCallFlags we can remove it in favor of _NONNULL helpers.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

---

If assertionprop sees that input object is already having a nonnull assertion we can give fgLateCastExpand a hint that we don't need a nullcheck, example:

// PGO warmupfor(inti=0;i<200;i++){Test(newProgram());Thread.Sleep(10);}[MethodImpl(MethodImplOptions.NoInlining)]voidTest(object?o){if(o!=null){if(oisProgram){Console.WriteLine();}}}

Diff: https://www.diffchecker.com/JkdxRigp/

Initially, I wanted to just introduce new _NONNULL cast helpers, but this change seems to be a lot smaller. In case if we run out of bits in gtMoreCallFlags in can be removed in favor of _NONNULL helpers.

Author:	EgorBo
Assignees:	EgorBo
Labels:	area-CodeGen-coreclr
Milestone:	-

[EgorBo]

@dotnet/jit-contrib @jakobbotsch PTAL

gtMoreFlags has 5 spare bits and this PR uses 1 of them. An alternative option is to introduce new fake *_NONNULL cast helpers - but that is a bit more changes + JIT-EE update (and R2R bump?)

Overall it looks like if we run out of spare bits we can move some rare things to side dictionaries or make some of them context-dependent. E.g. 3 GDV flags are only needed for virtual calls and only during early phases. GTF_CALL_M_EXP_RUNTIME_LOOKUP is only used in Debug and GTF_CALL_M_CAST_CAN_BE_EXPANDED will be hopefully removed next week.

Diffs aren't too big, but they will be a lot bigger once I move non-profiled (regular) casts to the late phase.

Build failures are known.

[jakobbotsch]

I definitely think having separate helpers would be better, especially since it would be slightly better for unprofiled casts too (assuming we would have cast variants that skip the null check inside them).

I'm ok with using a flag, but note that having flags that alter the semantics of nodes is IMO bad IR design. It's super easy to forget to check for. For example, shouldn't this PR check for the new flag inside GenTreeCall::Equals?

[EgorBo]

I don't have a strong preference here, I'll see what it will take to use the helpers, but I am guessing it's a copy-paste of all 8 helpers everywhere + R2R version bump + JIT-EE bump.

assuming we would have cast variants that skip the null check inside them

I thought about it too, but sounds like adding extra several kb to CastHelpers.cs to skip a nullcheck so unlikely worth the effort.

the semantics of nodes is IMO bad IR design

I agree generally, but looks like some of the existing flags already do similar things? e.g. tail call

[jakobbotsch]

I agree generally, but looks like some of the existing flags already do similar things? e.g. tail call

Yes, we have a number of flags that alter semantics, GTF_UNSIGNED being one of the big ones. I think GenTreeCall::Equals is also missing the checks for tail call flags (IIRC we talked about this one before).

I'm ok with merging this as is (with the GenTreeCall::Equals fix). If we think about a new representation it doesn't necessarily have to be via a different helper num if we do not think that's beneficial in the unprofiled case (in fact, I think it shouldn't be then). For example, we could have GT_ISCLASS, GT_ISCLASS_NONULL, GT_TLSLOOKUP, GT_RUNTIMELOOKUP when optimizing and create the GT_CALL only later.

[EgorBo]

@jakobbotsch I've fixed the GenTreeCall::Equals case + I addressed your other feedback regarding removing the debug only flag and gtGetEffectiveValue

[EgorBo]

Failure is #97049

[EgorBo]

If assertionprop sees that input object is already having a nonnull assertion we can give fgLateCastExpand a hint that we don't need a nullcheck, example:

// PGO warmupfor(inti=0;i<200;i++){Test(newProgram());Thread.Sleep(10);}[MethodImpl(MethodImplOptions.NoInlining)]voidTest(object?o){if(o!=null){if(oisProgram){Console.WriteLine();}}}

Diff: https://www.diffchecker.com/JkdxRigp/

Initially, I wanted to just introduce new _NONNULL cast helpers, but this change seems to be a lot smaller. In case if we run out of bits in gtMoreCallFlags we can remove it in favor of _NONNULL helpers.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

---

If assertionprop sees that input object is already having a nonnull assertion we can give fgLateCastExpand a hint that we don't need a nullcheck, example:

// PGO warmupfor(inti=0;i<200;i++){Test(newProgram());Thread.Sleep(10);}[MethodImpl(MethodImplOptions.NoInlining)]voidTest(object?o){if(o!=null){if(oisProgram){Console.WriteLine();}}}

Diff: https://www.diffchecker.com/JkdxRigp/

Initially, I wanted to just introduce new _NONNULL cast helpers, but this change seems to be a lot smaller. In case if we run out of bits in gtMoreCallFlags in can be removed in favor of _NONNULL helpers.

Author:	EgorBo
Assignees:	EgorBo
Labels:	area-CodeGen-coreclr
Milestone:	-

[EgorBo]

@dotnet/jit-contrib @jakobbotsch PTAL

gtMoreFlags has 5 spare bits and this PR uses 1 of them. An alternative option is to introduce new fake *_NONNULL cast helpers - but that is a bit more changes + JIT-EE update (and R2R bump?)

Overall it looks like if we run out of spare bits we can move some rare things to side dictionaries or make some of them context-dependent. E.g. 3 GDV flags are only needed for virtual calls and only during early phases. GTF_CALL_M_EXP_RUNTIME_LOOKUP is only used in Debug and GTF_CALL_M_CAST_CAN_BE_EXPANDED will be hopefully removed next week.

Diffs aren't too big, but they will be a lot bigger once I move non-profiled (regular) casts to the late phase.

Build failures are known.

[jakobbotsch]

I definitely think having separate helpers would be better, especially since it would be slightly better for unprofiled casts too (assuming we would have cast variants that skip the null check inside them).

I'm ok with using a flag, but note that having flags that alter the semantics of nodes is IMO bad IR design. It's super easy to forget to check for. For example, shouldn't this PR check for the new flag inside GenTreeCall::Equals?

[EgorBo]

I don't have a strong preference here, I'll see what it will take to use the helpers, but I am guessing it's a copy-paste of all 8 helpers everywhere + R2R version bump + JIT-EE bump.

assuming we would have cast variants that skip the null check inside them

I thought about it too, but sounds like adding extra several kb to CastHelpers.cs to skip a nullcheck so unlikely worth the effort.

the semantics of nodes is IMO bad IR design

I agree generally, but looks like some of the existing flags already do similar things? e.g. tail call

[jakobbotsch]

I agree generally, but looks like some of the existing flags already do similar things? e.g. tail call

Yes, we have a number of flags that alter semantics, GTF_UNSIGNED being one of the big ones. I think GenTreeCall::Equals is also missing the checks for tail call flags (IIRC we talked about this one before).

I'm ok with merging this as is (with the GenTreeCall::Equals fix). If we think about a new representation it doesn't necessarily have to be via a different helper num if we do not think that's beneficial in the unprofiled case (in fact, I think it shouldn't be then). For example, we could have GT_ISCLASS, GT_ISCLASS_NONULL, GT_TLSLOOKUP, GT_RUNTIMELOOKUP when optimizing and create the GT_CALL only later.

[EgorBo]

@jakobbotsch I've fixed the GenTreeCall::Equals case + I addressed your other feedback regarding removing the debug only flag and gtGetEffectiveValue

[EgorBo]

Failure is #97049

[EgorBo]

If assertionprop sees that input object is already having a nonnull assertion we can give fgLateCastExpand a hint that we don't need a nullcheck, example:

// PGO warmupfor(inti=0;i<200;i++){Test(newProgram());Thread.Sleep(10);}[MethodImpl(MethodImplOptions.NoInlining)]voidTest(object?o){if(o!=null){if(oisProgram){Console.WriteLine();}}}

Diff: https://www.diffchecker.com/JkdxRigp/

Initially, I wanted to just introduce new _NONNULL cast helpers, but this change seems to be a lot smaller. In case if we run out of bits in gtMoreCallFlags we can remove it in favor of _NONNULL helpers.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

---

If assertionprop sees that input object is already having a nonnull assertion we can give fgLateCastExpand a hint that we don't need a nullcheck, example:

// PGO warmupfor(inti=0;i<200;i++){Test(newProgram());Thread.Sleep(10);}[MethodImpl(MethodImplOptions.NoInlining)]voidTest(object?o){if(o!=null){if(oisProgram){Console.WriteLine();}}}

Diff: https://www.diffchecker.com/JkdxRigp/

Initially, I wanted to just introduce new _NONNULL cast helpers, but this change seems to be a lot smaller. In case if we run out of bits in gtMoreCallFlags in can be removed in favor of _NONNULL helpers.

Author:	EgorBo
Assignees:	EgorBo
Labels:	area-CodeGen-coreclr
Milestone:	-

[EgorBo]

@dotnet/jit-contrib @jakobbotsch PTAL

gtMoreFlags has 5 spare bits and this PR uses 1 of them. An alternative option is to introduce new fake *_NONNULL cast helpers - but that is a bit more changes + JIT-EE update (and R2R bump?)

Overall it looks like if we run out of spare bits we can move some rare things to side dictionaries or make some of them context-dependent. E.g. 3 GDV flags are only needed for virtual calls and only during early phases. GTF_CALL_M_EXP_RUNTIME_LOOKUP is only used in Debug and GTF_CALL_M_CAST_CAN_BE_EXPANDED will be hopefully removed next week.

Diffs aren't too big, but they will be a lot bigger once I move non-profiled (regular) casts to the late phase.

Build failures are known.

[jakobbotsch]

I definitely think having separate helpers would be better, especially since it would be slightly better for unprofiled casts too (assuming we would have cast variants that skip the null check inside them).

I'm ok with using a flag, but note that having flags that alter the semantics of nodes is IMO bad IR design. It's super easy to forget to check for. For example, shouldn't this PR check for the new flag inside GenTreeCall::Equals?

[EgorBo]

I don't have a strong preference here, I'll see what it will take to use the helpers, but I am guessing it's a copy-paste of all 8 helpers everywhere + R2R version bump + JIT-EE bump.

assuming we would have cast variants that skip the null check inside them

I thought about it too, but sounds like adding extra several kb to CastHelpers.cs to skip a nullcheck so unlikely worth the effort.

the semantics of nodes is IMO bad IR design

I agree generally, but looks like some of the existing flags already do similar things? e.g. tail call

[jakobbotsch]

I agree generally, but looks like some of the existing flags already do similar things? e.g. tail call

Yes, we have a number of flags that alter semantics, GTF_UNSIGNED being one of the big ones. I think GenTreeCall::Equals is also missing the checks for tail call flags (IIRC we talked about this one before).

I'm ok with merging this as is (with the GenTreeCall::Equals fix). If we think about a new representation it doesn't necessarily have to be via a different helper num if we do not think that's beneficial in the unprofiled case (in fact, I think it shouldn't be then). For example, we could have GT_ISCLASS, GT_ISCLASS_NONULL, GT_TLSLOOKUP, GT_RUNTIMELOOKUP when optimizing and create the GT_CALL only later.

[EgorBo]

@jakobbotsch I've fixed the GenTreeCall::Equals case + I addressed your other feedback regarding removing the debug only flag and gtGetEffectiveValue

[EgorBo]

Failure is #97049

[EgorBo]

If assertionprop sees that input object is already having a nonnull assertion we can give fgLateCastExpand a hint that we don't need a nullcheck, example:

// PGO warmupfor(inti=0;i<200;i++){Test(newProgram());Thread.Sleep(10);}[MethodImpl(MethodImplOptions.NoInlining)]voidTest(object?o){if(o!=null){if(oisProgram){Console.WriteLine();}}}

Diff: https://www.diffchecker.com/JkdxRigp/

Initially, I wanted to just introduce new _NONNULL cast helpers, but this change seems to be a lot smaller. In case if we run out of bits in gtMoreCallFlags we can remove it in favor of _NONNULL helpers.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

---

If assertionprop sees that input object is already having a nonnull assertion we can give fgLateCastExpand a hint that we don't need a nullcheck, example:

// PGO warmupfor(inti=0;i<200;i++){Test(newProgram());Thread.Sleep(10);}[MethodImpl(MethodImplOptions.NoInlining)]voidTest(object?o){if(o!=null){if(oisProgram){Console.WriteLine();}}}

Diff: https://www.diffchecker.com/JkdxRigp/

Initially, I wanted to just introduce new _NONNULL cast helpers, but this change seems to be a lot smaller. In case if we run out of bits in gtMoreCallFlags in can be removed in favor of _NONNULL helpers.

Author:	EgorBo
Assignees:	EgorBo
Labels:	area-CodeGen-coreclr
Milestone:	-

[EgorBo]

@dotnet/jit-contrib @jakobbotsch PTAL

gtMoreFlags has 5 spare bits and this PR uses 1 of them. An alternative option is to introduce new fake *_NONNULL cast helpers - but that is a bit more changes + JIT-EE update (and R2R bump?)

Overall it looks like if we run out of spare bits we can move some rare things to side dictionaries or make some of them context-dependent. E.g. 3 GDV flags are only needed for virtual calls and only during early phases. GTF_CALL_M_EXP_RUNTIME_LOOKUP is only used in Debug and GTF_CALL_M_CAST_CAN_BE_EXPANDED will be hopefully removed next week.

Diffs aren't too big, but they will be a lot bigger once I move non-profiled (regular) casts to the late phase.

Build failures are known.

[jakobbotsch]

I definitely think having separate helpers would be better, especially since it would be slightly better for unprofiled casts too (assuming we would have cast variants that skip the null check inside them).

I'm ok with using a flag, but note that having flags that alter the semantics of nodes is IMO bad IR design. It's super easy to forget to check for. For example, shouldn't this PR check for the new flag inside GenTreeCall::Equals?

[EgorBo]

I don't have a strong preference here, I'll see what it will take to use the helpers, but I am guessing it's a copy-paste of all 8 helpers everywhere + R2R version bump + JIT-EE bump.

assuming we would have cast variants that skip the null check inside them

I thought about it too, but sounds like adding extra several kb to CastHelpers.cs to skip a nullcheck so unlikely worth the effort.

the semantics of nodes is IMO bad IR design

I agree generally, but looks like some of the existing flags already do similar things? e.g. tail call

[jakobbotsch]

I agree generally, but looks like some of the existing flags already do similar things? e.g. tail call

Yes, we have a number of flags that alter semantics, GTF_UNSIGNED being one of the big ones. I think GenTreeCall::Equals is also missing the checks for tail call flags (IIRC we talked about this one before).

I'm ok with merging this as is (with the GenTreeCall::Equals fix). If we think about a new representation it doesn't necessarily have to be via a different helper num if we do not think that's beneficial in the unprofiled case (in fact, I think it shouldn't be then). For example, we could have GT_ISCLASS, GT_ISCLASS_NONULL, GT_TLSLOOKUP, GT_RUNTIMELOOKUP when optimizing and create the GT_CALL only later.

[EgorBo]

@jakobbotsch I've fixed the GenTreeCall::Equals case + I addressed your other feedback regarding removing the debug only flag and gtGetEffectiveValue

[EgorBo]

Failure is #97049

[EgorBo]

If assertionprop sees that input object is already having a nonnull assertion we can give fgLateCastExpand a hint that we don't need a nullcheck, example:

// PGO warmupfor(inti=0;i<200;i++){Test(newProgram());Thread.Sleep(10);}[MethodImpl(MethodImplOptions.NoInlining)]voidTest(object?o){if(o!=null){if(oisProgram){Console.WriteLine();}}}

Diff: https://www.diffchecker.com/JkdxRigp/

Initially, I wanted to just introduce new _NONNULL cast helpers, but this change seems to be a lot smaller. In case if we run out of bits in gtMoreCallFlags we can remove it in favor of _NONNULL helpers.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

---

If assertionprop sees that input object is already having a nonnull assertion we can give fgLateCastExpand a hint that we don't need a nullcheck, example:

// PGO warmupfor(inti=0;i<200;i++){Test(newProgram());Thread.Sleep(10);}[MethodImpl(MethodImplOptions.NoInlining)]voidTest(object?o){if(o!=null){if(oisProgram){Console.WriteLine();}}}

Diff: https://www.diffchecker.com/JkdxRigp/

Initially, I wanted to just introduce new _NONNULL cast helpers, but this change seems to be a lot smaller. In case if we run out of bits in gtMoreCallFlags in can be removed in favor of _NONNULL helpers.

Author:	EgorBo
Assignees:	EgorBo
Labels:	area-CodeGen-coreclr
Milestone:	-

[EgorBo]

@dotnet/jit-contrib @jakobbotsch PTAL

gtMoreFlags has 5 spare bits and this PR uses 1 of them. An alternative option is to introduce new fake *_NONNULL cast helpers - but that is a bit more changes + JIT-EE update (and R2R bump?)

Overall it looks like if we run out of spare bits we can move some rare things to side dictionaries or make some of them context-dependent. E.g. 3 GDV flags are only needed for virtual calls and only during early phases. GTF_CALL_M_EXP_RUNTIME_LOOKUP is only used in Debug and GTF_CALL_M_CAST_CAN_BE_EXPANDED will be hopefully removed next week.

Diffs aren't too big, but they will be a lot bigger once I move non-profiled (regular) casts to the late phase.

Build failures are known.

[jakobbotsch]

I definitely think having separate helpers would be better, especially since it would be slightly better for unprofiled casts too (assuming we would have cast variants that skip the null check inside them).

I'm ok with using a flag, but note that having flags that alter the semantics of nodes is IMO bad IR design. It's super easy to forget to check for. For example, shouldn't this PR check for the new flag inside GenTreeCall::Equals?

[EgorBo]

I don't have a strong preference here, I'll see what it will take to use the helpers, but I am guessing it's a copy-paste of all 8 helpers everywhere + R2R version bump + JIT-EE bump.

assuming we would have cast variants that skip the null check inside them

I thought about it too, but sounds like adding extra several kb to CastHelpers.cs to skip a nullcheck so unlikely worth the effort.

the semantics of nodes is IMO bad IR design

I agree generally, but looks like some of the existing flags already do similar things? e.g. tail call

[jakobbotsch]

I agree generally, but looks like some of the existing flags already do similar things? e.g. tail call

Yes, we have a number of flags that alter semantics, GTF_UNSIGNED being one of the big ones. I think GenTreeCall::Equals is also missing the checks for tail call flags (IIRC we talked about this one before).

I'm ok with merging this as is (with the GenTreeCall::Equals fix). If we think about a new representation it doesn't necessarily have to be via a different helper num if we do not think that's beneficial in the unprofiled case (in fact, I think it shouldn't be then). For example, we could have GT_ISCLASS, GT_ISCLASS_NONULL, GT_TLSLOOKUP, GT_RUNTIMELOOKUP when optimizing and create the GT_CALL only later.

[EgorBo]

@jakobbotsch I've fixed the GenTreeCall::Equals case + I addressed your other feedback regarding removing the debug only flag and gtGetEffectiveValue

[EgorBo]

Failure is #97049

[EgorBo]

If assertionprop sees that input object is already having a nonnull assertion we can give fgLateCastExpand a hint that we don't need a nullcheck, example:

// PGO warmupfor(inti=0;i<200;i++){Test(newProgram());Thread.Sleep(10);}[MethodImpl(MethodImplOptions.NoInlining)]voidTest(object?o){if(o!=null){if(oisProgram){Console.WriteLine();}}}

Diff: https://www.diffchecker.com/JkdxRigp/

Initially, I wanted to just introduce new _NONNULL cast helpers, but this change seems to be a lot smaller. In case if we run out of bits in gtMoreCallFlags we can remove it in favor of _NONNULL helpers.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

---

If assertionprop sees that input object is already having a nonnull assertion we can give fgLateCastExpand a hint that we don't need a nullcheck, example:

// PGO warmupfor(inti=0;i<200;i++){Test(newProgram());Thread.Sleep(10);}[MethodImpl(MethodImplOptions.NoInlining)]voidTest(object?o){if(o!=null){if(oisProgram){Console.WriteLine();}}}

Diff: https://www.diffchecker.com/JkdxRigp/

Initially, I wanted to just introduce new _NONNULL cast helpers, but this change seems to be a lot smaller. In case if we run out of bits in gtMoreCallFlags in can be removed in favor of _NONNULL helpers.

Author:	EgorBo
Assignees:	EgorBo
Labels:	area-CodeGen-coreclr
Milestone:	-

[EgorBo]

@dotnet/jit-contrib @jakobbotsch PTAL

gtMoreFlags has 5 spare bits and this PR uses 1 of them. An alternative option is to introduce new fake *_NONNULL cast helpers - but that is a bit more changes + JIT-EE update (and R2R bump?)

Overall it looks like if we run out of spare bits we can move some rare things to side dictionaries or make some of them context-dependent. E.g. 3 GDV flags are only needed for virtual calls and only during early phases. GTF_CALL_M_EXP_RUNTIME_LOOKUP is only used in Debug and GTF_CALL_M_CAST_CAN_BE_EXPANDED will be hopefully removed next week.

Diffs aren't too big, but they will be a lot bigger once I move non-profiled (regular) casts to the late phase.

Build failures are known.

[jakobbotsch]

I definitely think having separate helpers would be better, especially since it would be slightly better for unprofiled casts too (assuming we would have cast variants that skip the null check inside them).

I'm ok with using a flag, but note that having flags that alter the semantics of nodes is IMO bad IR design. It's super easy to forget to check for. For example, shouldn't this PR check for the new flag inside GenTreeCall::Equals?

[EgorBo]

I don't have a strong preference here, I'll see what it will take to use the helpers, but I am guessing it's a copy-paste of all 8 helpers everywhere + R2R version bump + JIT-EE bump.

assuming we would have cast variants that skip the null check inside them

I thought about it too, but sounds like adding extra several kb to CastHelpers.cs to skip a nullcheck so unlikely worth the effort.

the semantics of nodes is IMO bad IR design

I agree generally, but looks like some of the existing flags already do similar things? e.g. tail call

[jakobbotsch]

I agree generally, but looks like some of the existing flags already do similar things? e.g. tail call

Yes, we have a number of flags that alter semantics, GTF_UNSIGNED being one of the big ones. I think GenTreeCall::Equals is also missing the checks for tail call flags (IIRC we talked about this one before).

I'm ok with merging this as is (with the GenTreeCall::Equals fix). If we think about a new representation it doesn't necessarily have to be via a different helper num if we do not think that's beneficial in the unprofiled case (in fact, I think it shouldn't be then). For example, we could have GT_ISCLASS, GT_ISCLASS_NONULL, GT_TLSLOOKUP, GT_RUNTIMELOOKUP when optimizing and create the GT_CALL only later.

[EgorBo]

@jakobbotsch I've fixed the GenTreeCall::Equals case + I addressed your other feedback regarding removing the debug only flag and gtGetEffectiveValue

[EgorBo]

Failure is #97049