arm64: Add return cases into optimise bools #112945

jonathandavies-arm · 2025-02-26T09:40:03Z

Fixes JIT: simple range check is not branchless #96819

jonathandavies-arm · 2025-02-26T09:42:47Z

@a74nh @kunalspathak @dotnet/arm64-contrib

kunalspathak · 2025-02-28T02:10:06Z

src/coreclr/jit/optimizebools.cpp

@@ -1031,8 +1034,8 @@ bool OptBoolsDsc::optOptimizeCompareChainCondBlock()
        int op1Cost = cond1->GetCostEx();
        int op2Cost = cond2->GetCostEx();
        // The cost of combing three simple conditions is 32.
-        int maxOp1Cost = op1IsCondChain ? 31 : 7;
-        int maxOp2Cost = op2IsCondChain ? 31 : 7;
+        int maxOp1Cost = op1IsCondChain ? 31 : 10;


What are these changes?

If we don't increase the maxOpCost values then these changes won't really have much of an effect.
This is one of the tests I added.

[MethodImpl(MethodImplOptions.NoInlining)] static bool CmpOptimizeBoolsReturn(int a, int lower, int upper) { //ARM64-FULL-LINE: cmp {{w[0-9]+}}, {{w[0-9]+}} //ARM64-FULL-LINE: ccmp {{w[0-9]+}}, {{w[0-9]+}}, nzc, ge //ARM64-FULL-LINE: cset {{x[0-9]+}}, le return a >= lower && a <= upper; }

and when it gets to optOptimizeCompareChainCondBlock cond2 is this and it has a opCost of 8.

N003 ( 8, 5) [000008] -----+----- * EQ int $101 N001 ( 3, 2) [000006] -----+----- +--* LCL_VAR int V01 arg1 u:1 (last use) $81 N002 ( 1, 2) [000007] -----+----- \--* CNS_INT int 1 $41

I'm increasing these values so that these bool optimisations can also happen on return.

When I originally added these costs, they were just best guesses to ensures we don't optimise for long chains. Tweaking it should be fine.

kunalspathak · 2025-02-28T02:10:38Z

src/coreclr/jit/optimizebools.cpp

@@ -962,23 +962,26 @@ bool OptBoolsDsc::optOptimizeRangeTests()
 //
 bool OptBoolsDsc::optOptimizeCompareChainCondBlock()
 {
-    assert((m_b1 != nullptr) && (m_b2 != nullptr) && (m_b3 == nullptr));
+    assert((m_b1 != nullptr) && (m_b2 != nullptr));


As pointed by @jakobbotsch in #96819 (comment), should we still consider doing this based on the PGO information?

"That would require us to have reliable PGO data at the point of "optimize bools" though, which we don't have yet"

I think that's still the current state of PGO. So, not much we can do.

I actually think our PGO is in a good shape now (certainly much better than it was back then). cc @amanasifkhalid @AndyAyersMS for thoughts on using PGO information during optOptimizeBools

I think we ought to use it as a matter of principle, and identify cases where the PGO data fails us as motivation for introducing another profile repair call site somewhere midway through compilation. I don't think it's all that common for the profile data to be completely wrecked post-morph, and in the cases where it is, there is usually a bigger underlying problem we've yet to solve (i.e. method call counts for OSR methods), but I don't think that should block us from leveraging PGO data at all.

If we're going to use PGO in optimize bools, then it should be used in if conversion too, given they are fairly interlinked.

I'd like to suggest doing that in a separate PR, as I suspect it'll require some back and too in deciding what the exact cut off points are. Whereas this PR should only be causing diffs to happen in return cases.

I view if-conversion and this optimization as orthogonal optimizations: converting short circuiting conditionals into bitwise ops can be beneficial even on platforms that do not benefit from if conversion.

With that said, I am ok with not switching to utilize PGO information in this PR.

Added an issue here: #113080

(including if conversion, to save opening two issues)

kunalspathak · 2025-03-03T18:44:11Z

@jonathandavies-arm - there are bunch of failures in CI. Do you mind taking a look?

jakobbotsch · 2025-03-06T09:11:23Z

src/coreclr/jit/optimizebools.cpp

    }
    else
    {
-        return false;
+        GenTree* root = m_b3->firstStmt()->GetRootNode();
+        if (root->OperIsSimple())


What is this trying to test for? Why is this logic only necessary for unops/binops, and not any other node?

m_b3 is non null when block 2 first node is a return. In this case block 3 is also a return node. If block 3 returns a 0 then first condition (found in block 1) returns 0 when its condition is true. When we OR the 2 conditions together and NE them it will produce an incorrect result. So when block 3 returns 0 we need to inverse the first condition.

If we look at this function we can see what happens without this change.

[MethodImpl(MethodImplOptions.NoInlining)] private static bool GtAndGt(int x, int y) { return (x > 1 && y > 1); }

It will have these blocks when it enters the optimize bools phase

Before PHASE Optimize bools ------------ BB01 [0000] [000..004) -> BB03(0.5),BB02(0.5) (cond), preds={} succs={BB02,BB03} ***** BB01 [0000] STMT00000 ( 0x000[E-] ... 0x002 ) N004 ( 5, 6) [000003] -----+----- * JTRUE void $VN.Void N003 ( 3, 4) [000002] J----+-N--- \--* LE int $100 N001 ( 1, 1) [000000] -----+----- +--* LCL_VAR int V00 arg0 u:1 (last use) $80 N002 ( 1, 2) [000001] -----+----- \--* CNS_INT int 1 $41 ------------ BB02 [0001] [004..009) (return), preds={BB01} succs={} ***** BB02 [0001] STMT00002 ( 0x004[E-] ... 0x008 ) N004 ( 9, 6) [000009] -----+----- * RETURN int $VN.Void N003 ( 8, 5) [000008] -----+----- \--* GT int $101 N001 ( 3, 2) [000006] -----+----- +--* LCL_VAR int V01 arg1 u:1 (last use) $81 N002 ( 1, 2) [000007] -----+----- \--* CNS_INT int 1 $41 ------------ BB03 [0002] [009..00B) (return), preds={BB01} succs={} ***** BB03 [0002] STMT00001 ( 0x009[E-] ... 0x00A ) N002 ( 2, 3) [000005] -----+----- * RETURN int $VN.Void N001 ( 1, 2) [000010] -----+----- \--* CNS_INT int 0 $40 After PHASE Optimize bools ------------ BB01 [0000] [000..009) (return), preds={} succs={}

This shows the block after optimize bools without this else setting reverseFirstCondition.

***** BB01 [0000] STMT00002 ( 0x004[E-] ... 0x008 ) N010 ( 21, 14) [000009] -----+----- * RETURN int $VN.Void N009 ( 20, 13) [000013] ----------- \--* NE int N007 ( 15, 10) [000011] -------N--- +--* OR int N003 ( 6, 4) [000002] -----+-N--- | +--* LE int $100 N001 ( 1, 1) [000000] -----+----- | | +--* LCL_VAR int V00 arg0 u:1 (last use) $80 N002 ( 1, 2) [000001] -----+----- | | \--* CNS_INT int 1 $41 N006 ( 8, 5) [000008] -----+----- | \--* GT int $101 N004 ( 3, 2) [000006] -----+----- | +--* LCL_VAR int V01 arg1 u:1 (last use) $81 N005 ( 1, 2) [000007] -----+----- | \--* CNS_INT int 1 $41 N008 ( 1, 2) [000012] ----------- \--* CNS_INT int 0

And this show it with the else setting reverseFirstCondition

------------ BB01 [0000] [000..009) (return), preds={} succs={} ***** BB01 [0000] STMT00002 ( 0x004[E-] ... 0x008 ) N010 ( 21, 14) [000009] -----+----- * RETURN int $VN.Void N009 ( 20, 13) [000013] ----------- \--* NE int N007 ( 15, 10) [000011] -------N--- +--* AND int N003 ( 6, 4) [000002] -----+-N--- | +--* GT int N001 ( 1, 1) [000000] -----+----- | | +--* LCL_VAR int V00 arg0 u:1 (last use) $80 N002 ( 1, 2) [000001] -----+----- | | \--* CNS_INT int 1 $41 N006 ( 8, 5) [000008] -----+----- | \--* GT int $101 N004 ( 3, 2) [000006] -----+----- | +--* LCL_VAR int V01 arg1 u:1 (last use) $81 N005 ( 1, 2) [000007] -----+----- | \--* CNS_INT int 1 $41 N008 ( 1, 2) [000012] ----------- \--* CNS_INT int 0

Then I suggest replacing this root->OperIsSimple() by assert(root->OperIs(GT_RETURN)).

Also, I have noticed that the code is not set up to handle Swift returns correctly. This is preexisting, but can you please fix this while you are here? I do not think these optimizations should run for Swift returns since they do not validate anything about the error return.
The fix is just to remove GT_SIWFT_ERROR_RET in these lines:

runtime/src/coreclr/jit/optimizebools.cpp

Lines 1160 to 1172 in 78142b0

if (!testTree2->OperIs(GT_RETURN, GT_SWIFT_ERROR_RET))

{

return nullptr;

}

Statement* s3 = m_b3->firstStmt();

if (s3->GetPrevStmt() != s3)

{

return nullptr;

}

GenTree* testTree3 = s3->GetRootNode();

if (!testTree3->OperIs(GT_RETURN, GT_SWIFT_ERROR_RET))

The problem with changing this to an assert is that from running optboolsreturn.dll I see that we sometimes see a GT_SELECT_INV. Unfortunately this doesn't happen in a test and I can't see what code is reaching this code path.

This code needs to be moved to happen after optOptimizeBoolsChkBlkCond has been called, since that is validating that the shape of the IR allows the optimization to kick in. You can move the existing checks on the targets of the blocks as well.

…BoolsChkBlkCond()

jakobbotsch · 2025-03-07T12:39:26Z

src/coreclr/jit/optimizebools.cpp

    GenTree* cond2 = m_testInfo2.testTree->gtGetOp1();

+    if (m_b3 != nullptr)
+    {
+        GenTree* root = m_b3->firstStmt()->GetRootNode();


Suggested change

GenTree* root = m_b3->firstStmt()->GetRootNode();

assert(m_b1->FalseTargetIs(m_b2) && m_b1->TrueTargetIs(m_b3));

GenTree* root = m_b3->firstStmt()->GetRootNode();

arm64: Add return cases into optimise bools

226046d

* Fixes dotnet#96819

dotnet-issue-labeler bot added the area-CodeGen-coreclr label Feb 26, 2025

dotnet-policy-service bot added the community-contribution label Feb 26, 2025

jakobbotsch closed this Feb 26, 2025

jakobbotsch reopened this Feb 26, 2025

This was referenced Feb 26, 2025

slow macOS - "##[error]The job running on agent Azure Pipelines 9 ran longer than the maximum time of 60 minutes." dotnet/dnceng#1883

Open

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

kunalspathak reviewed Feb 28, 2025

View reviewed changes

a74nh mentioned this pull request Mar 3, 2025

Use PGO for Optimize Bools and If Conversion #113080

Open

Reverse first condition when 3rd block is returning 0

dcb022c

jakobbotsch reviewed Mar 6, 2025

View reviewed changes

Fix Cmp tests

3dd01c8

JulieLeeMSFT assigned kunalspathak and jonathandavies-arm Mar 6, 2025

JulieLeeMSFT added this to the 10.0.0 milestone Mar 6, 2025

Moved checking block 3 to reverse condition after calling optOptimize…

a3a81df

…BoolsChkBlkCond()

jakobbotsch reviewed Mar 7, 2025

View reviewed changes

assert block 2 & 3 are block 1's false & true targets respectively

dbbbcb9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

arm64: Add return cases into optimise bools #112945

arm64: Add return cases into optimise bools #112945

jonathandavies-arm commented Feb 26, 2025

jonathandavies-arm commented Feb 26, 2025

kunalspathak Feb 28, 2025

jonathandavies-arm Feb 28, 2025

a74nh Feb 28, 2025

kunalspathak Feb 28, 2025

a74nh Feb 28, 2025

jakobbotsch Feb 28, 2025

amanasifkhalid Feb 28, 2025 •

edited

Loading

a74nh Mar 3, 2025

jakobbotsch Mar 3, 2025

a74nh Mar 3, 2025

kunalspathak commented Mar 3, 2025

jakobbotsch Mar 6, 2025

jonathandavies-arm Mar 6, 2025

jakobbotsch Mar 6, 2025

jonathandavies-arm Mar 7, 2025

jakobbotsch Mar 7, 2025 •

edited

Loading

jakobbotsch Mar 7, 2025

	if (!testTree2->OperIs(GT_RETURN, GT_SWIFT_ERROR_RET))
	{
	return nullptr;
	}

	Statement* s3 = m_b3->firstStmt();
	if (s3->GetPrevStmt() != s3)
	{
	return nullptr;
	}

	GenTree* testTree3 = s3->GetRootNode();
	if (!testTree3->OperIs(GT_RETURN, GT_SWIFT_ERROR_RET))

	GenTree* root = m_b3->firstStmt()->GetRootNode();
	assert(m_b1->FalseTargetIs(m_b2) && m_b1->TrueTargetIs(m_b3));
	GenTree* root = m_b3->firstStmt()->GetRootNode();

arm64: Add return cases into optimise bools #112945

Are you sure you want to change the base?

arm64: Add return cases into optimise bools #112945

Conversation

jonathandavies-arm commented Feb 26, 2025

jonathandavies-arm commented Feb 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanasifkhalid Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunalspathak commented Mar 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch Mar 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanasifkhalid Feb 28, 2025 •

edited

Loading

jakobbotsch Mar 7, 2025 •

edited

Loading