Expand "return condition" into "return condition ? true : false"

[EgorBo]

Closes #8363
Closes #65370

Various control-flow related optimizations do not kick in for return <condition> patterns properly, because JIT mostly works with BBJ_COND types of blocks. optOptimizeBool is one of few phases which does support BBJ_RETURN but that support takes extra (a lot of) code to support.

Instead, let's just expand all return condition into return condition ? true : false and then convert back to return condition where profitable.

Example:

static bool Test(int x)
{
    return x == 10 || x == 20 || x == 30;
}

Here, "recognize bit-test" optimization (fgRecognizeSwitch) does not kick in due to BBJ_RETURN:

; Assembly listing for method Foo:Test(int):ubyte (FullOpts)
       cmp      ecx, 10
       je       SHORT G_M12679_IG04
       cmp      ecx, 20
       jne      SHORT G_M12679_IG06
G_M12679_IG04:
       mov      eax, 1
       ret      
G_M12679_IG06:
       cmp      ecx, 30
       sete     al
       movzx    rax, al
       ret      
; Total bytes of code 26

This PR:

; Assembly listing for method Foo:Test(int):ubyte (FullOpts)
       cmp      ecx, 30
       ja       SHORT G_M12679_IG06
       mov      eax, 0x3FEFFBFF
       bt       eax, ecx
       jb       SHORT G_M12679_IG06
       mov      eax, 1
       ret      
G_M12679_IG06:
       xor      eax, eax
       ret
; Total bytes of code 24

@stephentoub hit this problem when he was trying to showcase various jit optimizations on simple methods and had to rewrite them into if-else.

As a follow-up I might clean up various phases relying on BBJ_RETURN

[EgorBo]

@MihuBot

[EgorBo]

Will first address various size regressions in separate PRs before moving forward with this.

[xtqqczze]

@EgorBo Should this also close #65370?

[EgorBo]

PTAL @amanasifkhalid @AndyAyersMS cc @dotnet/jit-contrib
This PR does:

1. Expands BBJ_RETURN(cond) into BBJ_COND(cond), basically: return X == 42 into if (X == 42) { return true; } else { return false; } so downstream optimizations can focus only on BBJ_COND blocks. It allowed me to remove a lot of existing code from optimizebools.cpp as redundant (no size diffs from that).
2. Normally, tail-merging then merges all return true/false with other existing return true/false if any
3. After most opts are done, there is an attempt to merge BBJ_COND(cond) back to BBJ_RETURN(cond) via fgFoldCondToReturnBlock + ifConversion phase.

Seems to be nice PerfScore improvements: diffs (size diffs aren't too big, but PerfScore are). Many size regressions are PerfScore improvements, some are missing opportunities for ifConversion phase.

[amanasifkhalid]

Should there be an assert here?

[EgorBo]

Ah, oops, indeed

[amanasifkhalid]

retTrueBb and retFalseBb don't have any successors, right? To maintain the profile, I think you only need to decrease the weight of each return block by the weight of the edge you just removed. We have some helpers to do this:

if (block->hasProfileWeight())
{
    retTrueBb->decreaseBBProfileWeight(trueEdge->getLikelyWeight());
    retFalseBb->decreaseBBProfileWeight(falseEdge->getLikelyWeight());
}

If you know which epilogue is becoming unreachable, then you can skip this computation. Also, if you use fgRemoveAllRefPreds above instead of fgRemoveRefPred, it will return the removed edge, so you don't need to do this computation before changing block into a BBJ_RETURN.

Regardless of how you do it, I think we should be able to maintain the profile throughout this transformation.

[EgorBo]

retTrueBb and retFalseBb don't have any successors, right?

@amanasifkhalid one of them is allowed to have multiple predecessors, hence, can't be removed. But yeah, I agree that we should be able to set the weight, let me see

[EgorBo]

@amanasifkhalid have I addressed your feedback? in the recent commits

[amanasifkhalid]

Yes, just one more thing below

[amanasifkhalid]

nit: These BB weight helpers will set the BBF_PROF_WEIGHT flag, so if we always update the profile weights here, we will have a mix of profile-derived and heuristic-derived block weights in non-PGO scenarios. We've been trying to avoid this so far by wrapping these updates in if (block->hasProfileWeight()) checks. Some day, I hope to turn on profile synthesis for non-PGO scenarios too so we can get rid of these checks and always maintain the profile, but that will require us to start benchmarking NativeAOT regularly before I feel good about making that change.

In the meantime, could you please wrap this with a hasProfileWeight check? Thanks!

[EgorBo]

ah, I see - added checks

[AndyAyersMS]

Seems like we could proceed and then bail out later, if one of the successors is genReturnBB.

[EgorBo]

Although, I misunderstood how it works. If am not mistaken, when we run out of some arbitrary limit of RETURNs in the merge-return phase, we merge the rest using this genReturnBb so you're right, we should not give up here

[EgorBo]

Addressed via ce10e5c

[EgorBo]

/ba-g "azurelinux.3 timeous"