Roslyn: Compiler should optimize common boolean return patterns

[john-h-k]

Version Used:
Rosyln master branch from 2nd April 2019

Steps to Reproduce:
[[Sharplab Demo](https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQxASwDYB8ACAGAAhwEYBuAWACgcBmIgJgIGECBvKgzou4Ae14wEAygAteAVwwATAEJwA8gAcYaMGgBecKQApewAFZwAxjAK8AlGw5cbaAGYFdBNFDMHjMc9Zud2lHwFEAOwEMAjicBT+gQC+3j44IXYQGFCR8XHRnPG0BHwCBAAKCLyKiLruJmaWfoHBZs6ueoYmUTaZMUA)]

1. Have this code:
   public bool Proper(object o) { return o is object; }
2. Also, have this unoptimized version
   public bool ShouldBeOptimized(object o) { if (o is object) { return true; } return false; }

Expected Behavior:
Identical IL output, as they are semantically identical. This wouldn't be an issue but it does affect JIT output seemingly (JIT64 desktop, tested on Sharplab above) - it switches a setcc ; movzx pattern to a full branch, which is not ideal

Actual Behavior:
IL generation has an additional branch and changes the generated JIT

It is a very minor issue but was recommended to create an issue for it

[CyrusNajmabadi]

This feels like something the JIT should recognize and optimize. otherwise, every front-end language would need this optimization.

[john-h-k]

Turning a if (bool) return true else return false pattern into a return bool pattern feels like an optimization the frontend would be responsible because it such a simple one imo. However, it would be good for the JIT to recognise it yes

[CyrusNajmabadi]

Turning a if (bool) return true else return false pattern into a return bool pattern feels like an optimization the frontend would be responsible because it such a simple one imo.

I disagree. Front ends should do the analysis/optimziation for patterns that are too complex for the backend to do, or which require knowledge about teh constraints on the front end that hte back end doesn't know are there. For example, for patterns, we've said order of matching is undefined. That gives the front end a lot of options for optimizing things. Back-end wouldn't be able to do that because it doesn't know if that would break semantics the front-end expects.

ON the other hand, simple stuff is great for the backend. It doesn't cost much, woudl apply to all languages, and can be done safely.

However, it would be good for the JIT to recognise it yes

Agreed :D

[tannergooding]

(I know @CyrusNajmabadi and I have had this discussion in the past and didn't agree then, but I'll say it again here 😄)

While I agree it would be good for the backend to recognize; we will never be in a world where the JIT can do all of these optimizations. The JIT is a live compiler and has its own constraints; while the C# compiler is generally not live and is free to spend an appropriate amount of time on analysis/optimizations.

The JIT, due to it being live, has to make some trade-offs on what optimizations it recognizes and performs. Tiered compilation is going to improve it somewhat, but it still has to make some heuristic guesses about what will or won't be profitable. Often, these optimizations are simply based on things like "how many bytes of IL exist in the method I am about to compile" which can greatly impact things like inlining or how well it is able to analyze the overall method.

The primary downside to the C# compiler having these optimizations is that it also means that F#, VB, etc all need to implement similar optimizations. A good trade-off might be to have an intermediate ilopt.exe program that does these general-purpose optimizations. However, that itself comes with its own complexities such as also impacting PDBs (and debugging in general), sequence points, instrumentation, other post-IL rewriting programs, sourcelink, etc.

[CyrusNajmabadi]

A good trade-off might be to have an intermediate ilopt.exe program that does these general-purpose optimizations. However, that itself comes with its own complexities such as also impacting PDBs (and debugging in general), sequence points, instrumentation, other post-IL rewriting programs, sourcelink, etc.

I would be ok with that :)

The JIT, due to it being live, has to make some trade-offs on what optimizations it recognizes and performs.

True... though i would certainly hope test -> return true/false could fit into what it recognized :)

[CyrusNajmabadi]

(I know @CyrusNajmabadi and I have had this discussion in the past and didn't agree then, but I'll say it again here 😄)

<3

[gafter]

I'm putting this up for grabs. I think we'd take a solution as a community contribution as long as it isn't very complicated.

[john-h-k]

I can try and give this a shot. Should the transformation go in the LocalRewriter or in the emit optimizer? ( apparently I should tag you to ask 😄 @gafter )

[gafter]

@johnkellyoxford I have no idea. I suspect a change would have to cross some boundaries, but without experimenting I just don't know.