Break apart gtFoldExprConst into a couple functions so its more manageable

[tannergooding]

This should be zero-diffs (minus a minor fix for 32-bit xarch) as it's just taking the logic and splitting it out so we no longer have one gigantic method. This helps make the code easier to read/browse and should help the native compiler more easily inline the key paths in some cases.

Doing this refactoring since I'm planning on moving some more folding from morph into gtFoldExpr and this helps keep everything easy to understand.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

This PR refactors JIT constant folding by splitting gtFoldExprConst into unary, binary, type-specific, and bash/overflow helper methods. It is intended to preserve behavior while making the folding logic easier to navigate.

Changes:

• Routes unary and binary constant folding through new specialized helper methods.
• Extracts integer, long, floating-point, and overflow folding logic into separate functions.
• Adds helper declarations for the new folding and constant-bashing routines.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.

File	Description
src/coreclr/jit/gentree.cpp	Refactors constant folding implementation into smaller helpers.
src/coreclr/jit/compiler.h	Declares the new constant-folding helper methods.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

[tannergooding]

CC. @dotnet/jit-contrib, @EgorBo

as per the top post, this just takes gtFoldExprConst and splits it apart into a few clear functions based on unary vs binary and the tree type it’s handling (int vs long vs float/double).

This makes each path overall smaller and minimally helps throughput (< https://dev.azure.com/dnceng-public/public/_build/results?buildId=1424088&view=ms.vss-build-web.run-extensions-tab>) giving up to -0.10% in MinOpts and 0 asm-diffs

The code being one giant function with many gotos was impacting native compilers negatively and made it hard to see what was doing what

There’s a few floating-point cases being pessimized (not handling things they trivially could handle) that don’t need to be, but those are revisiting so I’ll do the functionality changes in a follow up pr

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

[EgorBo]

To be honest, it's not entirely clear for me what this PR does (besides adding +400 LOC) is there a break down? All i see are very similiarly looking functions (that probably could be template-ized), @dotnet/jit-contrib for opinions

[tannergooding]

To be honest, it's not entirely clear for me what this PR does (besides adding +400 LOC) is there a break dow

As in the top post and the tag, it takes gtFoldExprConst which was previously one giant function handling all of unary, binary, int, long, float, double, etc, with a large number of goto, and splits it into distinct functions for each. -- So we have gtFoldExprUnaryConst and gtFoldExprUnaryConstInt, to avoid having a function that is "too big" and which clearly separates each important scenario without being too granular.

This makes it easier to follow the control flow (for both human and compiler), makes it easier to compare/contrast the paths, makes it so the compiler can more easily do inlining and DCE, etc.

The +400 LOC is primarily the method header comments and braces added; particularly as I made effort to ensure this was just a refactoring (aside from the 32-bit xarch case for floating-point that didn't need to differ anymore), It gives a nice TP win because the code is explicitly less complex now.

that probably could be template-ized

Same as with the other case. They are similar, but often doing quite distinct things at various points, including in the logic needed for ensuring it is not relying on undefined or implementation defined behavior.

The places where they are most similar tend to be one liners, so you're not ending up with much practical sharing and make the code more complex otherwise; and with a higher risk of things going incorrectly if users pass in the wrong type (such as using ssize_t instead of int32_t, or accidentally passing in a small type, etc).

[EgorBo]

The +400 LOC is primarily the method header comments and braces added; particularly as I made effort to ensure this was just a refactoring (aside from the 32-bit xarch case for floating-point that didn't need to differ anymore), It gives a nice TP win because the code is explicitly less complex now.

I think a fraction of TP improvements is the least important factor when we talk about maintainability. Overall, I'm fine with these changes (but still think this area didn't have any maintainability issues before to add extra +400 LOC - it ruins muscle memory while there are many other places that do require some clean up like morph) unless @dotnet/jit-contrib disagrees.

I think such refactorings should solve concrete issues e.g. "we used to introduce bugs because there were multiple places to update, but not anymore". And more importantly, I'd love to see code reduction, not adding more.

[tannergooding]

I think a fraction of TP improvements is the least important factor when we talk about maintainability

Agreed, but in this case its removing explicitly convoluted code involving a giant method, intermixed state, and a bunch of gotos causing complex control flow for something that is very simple, linear, and straightforward.

The additional lines are not "extra code" to maintain, they are primarily the boilerplate such as method headers, explicit { } braces around scopes, and method asserts for entry invariants. The things we actively know help avoid bugs and help with maintainability and where the prior code was missing it. -- In other words, they are effectively punctuation rather than logic.

And more importantly, I'd love to see code reduction, not adding more.

I expect in many cases, especially as it goes to cleaning up functions and making it more maintainable, we will be explicitly adding lines. This will namely come from legacy code having bad practices which are part of what it complex and error prone and the new code following consistent standards which increase the lines while reducing the things to think about.

It's also about taking the incremental steps to get towards eventual reductions. We cannot be mixing big refactorings like this with functionality changes. The end goal here is to get most of the actual folding operations centralized so they aren't duplicated between here and morph, but moving that logic down has much broader impact. So I split it into:

1. Get the code in a format that allows easier moving of the logic down and ensuring it is correct
2. A follow up PR that actually moves the folding logic out of morph and into here