Fold null checks against known non-null values #109164

MichalPetryka · 2024-10-23T20:17:03Z

Found in #108579.

Found in dotnet#108579.

dotnet-policy-service · 2024-10-23T20:17:34Z

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

MichalPetryka · 2024-10-23T20:18:13Z

@MihuBot

MichalPetryka · 2024-10-23T20:43:53Z

@MihuBot -dependsOn 108579

src/coreclr/jit/gentree.cpp

EgorBo · 2024-10-24T09:40:30Z

src/coreclr/jit/gentree.cpp


-                        return NewMorphedIntConNode(compareResult);
+                    GenTree* newTree = gtNewIconNode(compareResult);
+                    if (wrapEffects)


just wrap unconditionally, gtWrapWithSideEffects won't create a COMMA if there are no side-effects

AFAIR we can't use the op if gtTryRemoveBoxUpstreamEffects removes the box, that's why I did the check.

not sure I understand, if it removes - does it leave a tree with a side-effect?

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.

EgorBo · 2024-10-24T09:41:48Z

src/coreclr/jit/gentree.cpp

-                    GenTree* boxSourceTree = gtTryRemoveBoxUpstreamEffects(op);
-                    bool     didOptimize   = (boxSourceTree != nullptr);
+                        // See if we can optimize away the box and related statements.
+                        wrapEffects = (gtTryRemoveBoxUpstreamEffects(op) == nullptr);


previous logic used to give up if box can't be removed, is it expected that the new one always folds?

btw, does it all handle boxed nullables (when boxed value is null reference)?

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.

dotnet#109164 (comment)

MichalPetryka · 2024-11-21T21:40:05Z

@MihuBot -dependsOn 109715

MichalPetryka · 2024-11-22T19:51:55Z

@jkoritzinsky Windows jit-format job appears to be broken, could you look into this?

@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?

2024-11-21T21:51:02.9088540Z   clang: error: unable to execute command: Abort trap: 6
2024-11-21T21:51:02.9714770Z   clang: error: clang frontend command failed due to signal (use -v to see invocation)
2024-11-21T21:51:03.0717300Z   Apple clang version 15.0.0 (clang-1500.1.0.2.5)
2024-11-21T21:51:03.1719340Z   Target: x86_64-apple-darwin22.6.0
2024-11-21T21:51:03.2721070Z   Thread model: posix
2024-11-21T21:51:03.3616560Z   InstalledDir: /Applications/Xcode_15.2.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
2024-11-21T21:51:03.5356310Z   [ 75%] Building CXX object jit/CMakeFiles/clrjit_universal_arm64_x64.dir/ifconversion.cpp.o
2024-11-21T21:51:03.6593320Z   [ 75%] Building CXX object jit/CMakeFiles/clrjit_universal_arm64_x64.dir/importer.cpp.o
2024-11-21T21:51:03.7539170Z   [ 75%] Building CXX object jit/CMakeFiles/clrjit_universal_arm64_x64.dir/importercalls.cpp.o
2024-11-21T21:51:03.8653490Z   clang: note: diagnostic msg: 
2024-11-21T21:51:03.9656260Z   ********************
2024-11-21T21:51:04.0658900Z   
2024-11-21T21:51:04.1044960Z   PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
2024-11-21T21:51:04.1394990Z   Preprocessed source(s) and associated run script(s) are located at:
2024-11-21T21:51:04.1624610Z   clang: note: diagnostic msg: /var/folders/p1/44pzfl0j2m301zzfb0fv0p380000gn/T/hashbv-f258fd.cpp
2024-11-21T21:51:04.2044370Z   clang: note: diagnostic msg: /var/folders/p1/44pzfl0j2m301zzfb0fv0p380000gn/T/hashbv-f258fd.sh
2024-11-21T21:51:04.2325410Z   clang: note: diagnostic msg: Crash backtrace is located in
2024-11-21T21:51:04.2550480Z   clang: note: diagnostic msg: /Users/runner/Library/Logs/DiagnosticReports/clang_<YYYY-MM-DD-HHMMSS>_<hostname>.crash
2024-11-21T21:51:04.2557650Z   clang: note: diagnostic msg: (choose the .crash file that corresponds to your crash)
2024-11-21T21:51:04.2563810Z   clang: note: diagnostic msg: 
2024-11-21T21:51:04.2570240Z   
2024-11-21T21:51:04.2585420Z   ********************
2024-11-21T21:51:04.2590510Z   make[3]: *** [jit/CMakeFiles/clrjit_universal_arm64_x64.dir/hashbv.cpp.o] Error 1
2024-11-21T21:51:04.2597660Z   make[3]: *** Waiting for unfinished jobs....
2024-11-21T21:51:08.5169240Z   make[2]: *** [jit/CMakeFiles/clrjit_universal_arm64_x64.dir/all] Error 2
2024-11-21T21:51:08.5182910Z   make[1]: *** [CMakeFiles/crosscomponents.dir/rule] Error 2
2024-11-21T21:51:08.5186860Z   make: *** [crosscomponents] Error 2
2024-11-21T21:51:08.5188920Z   ~/work/1/s/src/coreclr
2024-11-21T21:51:08.5228330Z ##[error]Failed to build "CoreCLR component".

jkotas · 2024-11-22T20:00:42Z

@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?

MichalPetryka · 2024-11-22T20:04:10Z

@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?

jkotas · 2024-11-22T20:30:51Z

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.

MichalPetryka · 2024-11-22T22:31:31Z

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.

EgorBo · 2024-12-16T13:27:01Z

src/coreclr/jit/flowgraph.cpp

@@ -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();


remind me why GT_BOX may produce null possibly? I don't understand the need in IsBoxedValue()

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.

Fold null checks against known non-null values

c76e4e1

Found in dotnet#108579.

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 23, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Oct 23, 2024

MihuBot mentioned this pull request Oct 23, 2024

[JitDiff X64] [MichalPetryka] Fold null checks against known non-null values MihuBot/runtime-utils#750

Open

MichalPetryka added 2 commits October 23, 2024 22:39

Update gentree.cpp

90b0762

Update gentree.cpp

560a826

MihuBot mentioned this pull request Oct 23, 2024

[JitDiff X64] [MichalPetryka] Fold null checks against known non-null values MihuBot/runtime-utils#751

Open

EgorBo reviewed Oct 24, 2024

View reviewed changes

src/coreclr/jit/gentree.cpp Outdated Show resolved Hide resolved

EgorBo reviewed Oct 24, 2024

View reviewed changes

MichalPetryka added a commit to MichalPetryka/runtime that referenced this pull request Oct 25, 2024

Check for boxes in fgAddrCouldBeNull

124c7a8

dotnet#109164 (comment)

MichalPetryka mentioned this pull request Oct 25, 2024

Check for boxes in fgAddrCouldBeNull #109208

Closed

MichalPetryka added 2 commits October 28, 2024 16:12

Update flowgraph.cpp

9260b88

Update gentree.cpp

365b604

JulieLeeMSFT requested a review from EgorBo November 18, 2024 18:46

JulieLeeMSFT assigned EgorBo and MichalPetryka Nov 18, 2024

Merge branch 'main' into patch-43

fddb0b3

MihuBot mentioned this pull request Nov 21, 2024

[JitDiff X64] [MichalPetryka] Fold null checks against known non-null values MihuBot/runtime-utils#791

Open

Merge branch 'main' into patch-43

882ba2c

Merge branch 'main' into patch-43

ebf1479

EgorBo reviewed Dec 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fold null checks against known non-null values #109164

Fold null checks against known non-null values #109164

MichalPetryka commented Oct 23, 2024

dotnet-policy-service bot commented Oct 23, 2024

MichalPetryka commented Oct 23, 2024

MichalPetryka commented Oct 23, 2024

EgorBo Oct 24, 2024

MichalPetryka Oct 24, 2024

EgorBo Oct 25, 2024

MichalPetryka Nov 21, 2024

EgorBo Oct 24, 2024

EgorBo Oct 24, 2024

MichalPetryka Oct 24, 2024 •

edited

Loading

MichalPetryka commented Nov 21, 2024

MichalPetryka commented Nov 22, 2024 •

edited

Loading

jkotas commented Nov 22, 2024

MichalPetryka commented Nov 22, 2024

jkotas commented Nov 22, 2024

MichalPetryka commented Nov 22, 2024

EgorBo Dec 16, 2024

MichalPetryka Dec 16, 2024 •

edited

Loading

Fold null checks against known non-null values #109164

Are you sure you want to change the base?

Fold null checks against known non-null values #109164

Conversation

MichalPetryka commented Oct 23, 2024

dotnet-policy-service bot commented Oct 23, 2024

MichalPetryka commented Oct 23, 2024

MichalPetryka commented Oct 23, 2024

EgorBo Oct 24, 2024

Choose a reason for hiding this comment

MichalPetryka Oct 24, 2024

Choose a reason for hiding this comment

EgorBo Oct 25, 2024

Choose a reason for hiding this comment

MichalPetryka Nov 21, 2024

Choose a reason for hiding this comment

EgorBo Oct 24, 2024

Choose a reason for hiding this comment

EgorBo Oct 24, 2024

Choose a reason for hiding this comment

MichalPetryka Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

MichalPetryka commented Nov 21, 2024

MichalPetryka commented Nov 22, 2024 • edited Loading

jkotas commented Nov 22, 2024

MichalPetryka commented Nov 22, 2024

jkotas commented Nov 22, 2024

MichalPetryka commented Nov 22, 2024

EgorBo Dec 16, 2024

Choose a reason for hiding this comment

MichalPetryka Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

MichalPetryka Oct 24, 2024 •

edited

Loading

MichalPetryka commented Nov 22, 2024 •

edited

Loading

MichalPetryka Dec 16, 2024 •

edited

Loading