-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fold null checks against known non-null values #109164
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
@MihuBot -dependsOn 108579 |
src/coreclr/jit/gentree.cpp
Outdated
|
||
return NewMorphedIntConNode(compareResult); | ||
GenTree* newTree = gtNewIconNode(compareResult); | ||
if (wrapEffects) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just wrap unconditionally, gtWrapWithSideEffects
won't create a COMMA if there are no side-effects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIR we can't use the op
if gtTryRemoveBoxUpstreamEffects
removes the box, that's why I did the check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure I understand, if it removes - does it leave a tree with a side-effect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure I understand, if it removes - does it leave a tree with a side-effect?
As far as @SingleAccretion explained it to me on Discord, using the tree when gtTryRemoveBoxUpstreamEffects
succeedes is nonsensical since the original tree is invalid due to the earlier box not existing anymore.
src/coreclr/jit/gentree.cpp
Outdated
GenTree* boxSourceTree = gtTryRemoveBoxUpstreamEffects(op); | ||
bool didOptimize = (boxSourceTree != nullptr); | ||
// See if we can optimize away the box and related statements. | ||
wrapEffects = (gtTryRemoveBoxUpstreamEffects(op) == nullptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
previous logic used to give up if box can't be removed, is it expected that the new one always folds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, does it all handle boxed nullables (when boxed value is null reference)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it expected that the new one always folds?
Yeah, my reasoning here was that there's no reason to avoid folding if we can't remove.
does it all handle boxed nullables (when boxed value is null reference)?
Not sure, I'd assume those wouldn't pass IsBoxedValue
, otherwise I'd think the previous code would be broken.
EDIT: They wouldn't, IsBoxedValue
checks GTF_BOX_VALUE
which guarantees it's not null.
@MihuBot -dependsOn 109715 |
@jkoritzinsky Windows @jkotas It seems that there was some Apple Clang bug that manifested when building a file I didn't touch here, do you know who'd be the right person for handling this?
|
Is it deterministic repro? |
I don't see it failing on other PRs so I assume no, should I merge master again and see if it fails again? |
Yes. The temp files that the error messages suggest collecting are gone, so nothing will be lost by retrying. |
Seems it didn't happen again. |
@@ -863,6 +863,9 @@ bool Compiler::fgAddrCouldBeNull(GenTree* addr) | |||
case GT_ARR_ADDR: | |||
return (addr->gtFlags & GTF_ARR_ADDR_NONNULL) == 0; | |||
|
|||
case GT_BOX: | |||
return !addr->IsBoxedValue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remind me why GT_BOX may produce null possibly? I don't understand the need in IsBoxedValue()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IsBoxedValue
checks the GTF_BOX_VALUE
flag which is documented as guaranteeing that the box is not null, when we discussed this on Discord we reached the conclusion that the flag might be an old leftover that's not needed anymore and could be removed, but like I said there, I'd prefer such cleanup to be a separate PR.
@EgorBo, please review this PR. |
* main: (27 commits) Fold null checks against known non-null values (dotnet#109164) JIT: Always track the context for late devirt (dotnet#112396) JIT: array allocation fixes (dotnet#112676) [H/3] Fix test closing connection too fast (dotnet#112691) Fix LINQ handling of iterator.Take(...).Last(...) (dotnet#112680) [browser][MT] move wasm MT CI legs to extra-platforms (dotnet#112690) JIT: Don't use `Compiler::compFloatingPointUsed` to check if FP kills are needed (dotnet#112668) [LoongArch64] Fix a typo within PR#112166. (dotnet#112672) Fix new EH hang on DebugBreak (dotnet#112640) Use encode callback instead of renting a buffer to write to in DSAKeyFormatHelper Move some links to other doc (dotnet#112574) Reflection-based XmlSerializer - Deserialize empty collections and allow for sub-types in collection items. (dotnet#111723) JIT: Use `fgCalledCount` for OSR method entry weight (dotnet#112662) Use Avx10.2 Instructions in Floating Point Conversions (dotnet#111775) Expose StressLog via CDAC and port StressLogAnalyzer to managed code (dotnet#104999) JIT: Use linear block order for MinOpts in LSRA (dotnet#108147) Update dependencies from https://github.com/dotnet/arcade build 20250213.2 (dotnet#112625) JIT: Clean up and optimize call arg lowering (dotnet#112639) Update dependencies from https://github.com/dotnet/emsdk build 20250217.1 (dotnet#112645) JIT: Support `FIELD_LIST` for returns (dotnet#112308) ...
Found in #108579.