[SimplifyCFG] Forward indirect switch condition value if it can help fold the PHI #95932

dianqk · 2024-06-18T14:13:16Z

Fixes #95919.

llvmbot · 2024-06-18T14:13:51Z

@llvm/pr-subscribers-backend-aarch64

@llvm/pr-subscribers-llvm-transforms

Author: DianQK (DianQK)

Changes

Fixes #95919.

To me, this limit doesn't make sense.

Full diff: https://github.com/llvm/llvm-project/pull/95932.diff

4 Files Affected:

(modified) llvm/lib/Transforms/Utils/SimplifyCFG.cpp (-2)
(modified) llvm/test/CodeGen/AArch64/arm64-jumptable.ll (+1-1)
(modified) llvm/test/Transforms/SimplifyCFG/ForwardSwitchConditionToPHI.ll (+69)
(modified) llvm/test/Transforms/SimplifyCFG/Hexagon/switch-to-lookup-table.ll (+2-2)

diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
index 4e2dc7f2b2f4e..5a4c258937a27 100644
--- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -5818,8 +5818,6 @@ static bool ForwardSwitchConditionToPHI(SwitchInst *SI) {
   for (auto &ForwardingNode : ForwardingNodes) {
     PHINode *Phi = ForwardingNode.first;
     SmallVectorImpl<int> &Indexes = ForwardingNode.second;
-    if (Indexes.size() < 2)
-      continue;
 
     for (int Index : Indexes)
       Phi->setIncomingValue(Index, SI->getCondition());
diff --git a/llvm/test/CodeGen/AArch64/arm64-jumptable.ll b/llvm/test/CodeGen/AArch64/arm64-jumptable.ll
index 7d9adf92a6a95..40b8f721c0505 100644
--- a/llvm/test/CodeGen/AArch64/arm64-jumptable.ll
+++ b/llvm/test/CodeGen/AArch64/arm64-jumptable.ll
@@ -18,7 +18,7 @@ bb3:
 bb4:
   br label %exit.sink.split
 exit.sink.split:
-  %.sink = phi i32 [ 5, %bb4 ], [ %b, %bb1 ], [ 3, %bb3 ], [ %a, %entry ]
+  %.sink = phi i32 [ 5, %bb4 ], [ %b, %bb1 ], [ 7, %bb3 ], [ %a, %entry ]
   store i32 %.sink, ptr %to
   br label %exit
 exit:
diff --git a/llvm/test/Transforms/SimplifyCFG/ForwardSwitchConditionToPHI.ll b/llvm/test/Transforms/SimplifyCFG/ForwardSwitchConditionToPHI.ll
index 5c266f4867398..5e00a4b098f68 100644
--- a/llvm/test/Transforms/SimplifyCFG/ForwardSwitchConditionToPHI.ll
+++ b/llvm/test/Transforms/SimplifyCFG/ForwardSwitchConditionToPHI.ll
@@ -59,6 +59,75 @@ return:                                           ; preds = %entry, %sw.bb4, %sw
   ret i32 %retval.0
 }
 
+define i32 @forward_one(i32 %m) {
+; NO_FWD-LABEL: @forward_one(
+; NO_FWD-NEXT:  entry:
+; NO_FWD-NEXT:    switch i32 [[M:%.*]], label [[SW_BB4:%.*]] [
+; NO_FWD-NEXT:      i32 0, label [[RETURN:%.*]]
+; NO_FWD-NEXT:      i32 1, label [[SW_BB1:%.*]]
+; NO_FWD-NEXT:      i32 2, label [[SW_BB2:%.*]]
+; NO_FWD-NEXT:      i32 3, label [[SW_BB3:%.*]]
+; NO_FWD-NEXT:    ]
+; NO_FWD:       sw.bb1:
+; NO_FWD-NEXT:    br label [[RETURN]]
+; NO_FWD:       sw.bb2:
+; NO_FWD-NEXT:    br label [[RETURN]]
+; NO_FWD:       sw.bb3:
+; NO_FWD-NEXT:    br label [[RETURN]]
+; NO_FWD:       sw.bb4:
+; NO_FWD-NEXT:    br label [[RETURN]]
+; NO_FWD:       return:
+; NO_FWD-NEXT:    [[RETVAL_0:%.*]] = phi i32 [ 4, [[SW_BB4]] ], [ 5, [[SW_BB3]] ], [ 6, [[SW_BB2]] ], [ 1, [[SW_BB1]] ], [ 8, [[ENTRY:%.*]] ]
+; NO_FWD-NEXT:    ret i32 [[RETVAL_0]]
+;
+; FWD-LABEL: @forward_one(
+; FWD-NEXT:  entry:
+; FWD-NEXT:    switch i32 [[M:%.*]], label [[SW_BB4:%.*]] [
+; FWD-NEXT:      i32 0, label [[RETURN:%.*]]
+; FWD-NEXT:      i32 1, label [[SW_BB1:%.*]]
+; FWD-NEXT:      i32 2, label [[SW_BB2:%.*]]
+; FWD-NEXT:      i32 3, label [[SW_BB3:%.*]]
+; FWD-NEXT:    ]
+; FWD:       sw.bb1:
+; FWD-NEXT:    br label [[RETURN]]
+; FWD:       sw.bb2:
+; FWD-NEXT:    br label [[RETURN]]
+; FWD:       sw.bb3:
+; FWD-NEXT:    br label [[RETURN]]
+; FWD:       sw.bb4:
+; FWD-NEXT:    br label [[RETURN]]
+; FWD:       return:
+; FWD-NEXT:    [[RETVAL_0:%.*]] = phi i32 [ 4, [[SW_BB4]] ], [ 5, [[SW_BB3]] ], [ 6, [[SW_BB2]] ], [ [[M]], [[SW_BB1]] ], [ 8, [[ENTRY:%.*]] ]
+; FWD-NEXT:    ret i32 [[RETVAL_0]]
+;
+entry:
+  switch i32 %m, label %sw.bb4 [
+  i32 0, label %sw.bb0
+  i32 1, label %sw.bb1
+  i32 2, label %sw.bb2
+  i32 3, label %sw.bb3
+  ]
+
+sw.bb0:                                           ; preds = %entry
+  br label %return
+
+sw.bb1:                                           ; preds = %entry
+  br label %return
+
+sw.bb2:                                           ; preds = %entry
+  br label %return
+
+sw.bb3:                                           ; preds = %entry
+  br label %return
+
+sw.bb4:                                           ; preds = %entry
+  br label %return
+
+return:                                           ; preds = %entry, %sw.bb4, %sw.bb3, %sw.bb2, %sw.bb1
+  %retval.0 = phi i32 [ 4, %sw.bb4 ], [ 5, %sw.bb3 ], [ 6, %sw.bb2 ], [ 1, %sw.bb1 ], [ 8, %sw.bb0 ]
+  ret i32 %retval.0
+}
+
 ; If 1 incoming phi value is a case constant of a switch, convert it to the switch condition:
 ; https://bugs.llvm.org/show_bug.cgi?id=34471
 ; This then subsequently should allow squashing of the other trivial case blocks.
diff --git a/llvm/test/Transforms/SimplifyCFG/Hexagon/switch-to-lookup-table.ll b/llvm/test/Transforms/SimplifyCFG/Hexagon/switch-to-lookup-table.ll
index 47dd24e194c39..b3b6abe314e6f 100644
--- a/llvm/test/Transforms/SimplifyCFG/Hexagon/switch-to-lookup-table.ll
+++ b/llvm/test/Transforms/SimplifyCFG/Hexagon/switch-to-lookup-table.ll
@@ -43,7 +43,7 @@ define i32 @foo(i32 %x) #0 section ".tcm_text" {
 ; DISABLE:       sw.default:
 ; DISABLE-NEXT:    br label [[RETURN]]
 ; DISABLE:       return:
-; DISABLE-NEXT:    [[RETVAL_0:%.*]] = phi i32 [ 19, [[SW_DEFAULT]] ], [ 5, [[SW_BB5]] ], [ 12, [[SW_BB4]] ], [ 22, [[SW_BB3]] ], [ 14, [[SW_BB2]] ], [ 20, [[SW_BB1]] ], [ 9, [[ENTRY:%.*]] ]
+; DISABLE-NEXT:    [[RETVAL_0:%.*]] = phi i32 [ 19, [[SW_DEFAULT]] ], [ 33, [[SW_BB5]] ], [ 12, [[SW_BB4]] ], [ 22, [[SW_BB3]] ], [ 14, [[SW_BB2]] ], [ 20, [[SW_BB1]] ], [ 9, [[ENTRY:%.*]] ]
 ; DISABLE-NEXT:    ret i32 [[RETVAL_0]]
 ;
 entry:
@@ -81,7 +81,7 @@ sw.bb4:                                           ; preds = %entry
   br label %return
 
 sw.bb5:                                           ; preds = %entry
-  store i32 5, ptr %retval, align 4
+  store i32 33, ptr %retval, align 4
   br label %return
 
 sw.default:                                       ; preds = %entry

nikic

Is it possible to also add a test case where this shows an actual benefit like in #95919? Or does this require multiple passes?

…itch condition

dianqk · 2024-06-18T14:41:17Z

Is it possible to also add a test case where this shows an actual benefit like in #95919? Or does this require multiple passes?

Yes, the existing -switch-range-to-icmp is already sufficient.

llvm/test/CodeGen/AArch64/arm64-jumptable.ll

PR Link: llvm/llvm-project#95932

dtcxzyw

LGTM. Thank you!

nikic

LGTM

zmodem · 2024-06-19T08:21:19Z

To me, this limit doesn't make sense.

Looks like I added this in 4ab4a8e

I think the point of the Indexes.size() check is explained by the "if that would mean that some of the destination blocks of the switch can be folded away" part of this comment:

llvm-project/llvm/lib/Transforms/Utils/SimplifyCFG.cpp

Lines 5774 to 5776 in 9cbedd9

    
           /// Try to forward the condition of a switch instruction to a phi node 
        
           /// dominated by the switch, if that would mean that some of the destination 
        
           /// blocks of the switch can be folded away. Return true if a change is made.

The point of the forwarding is to make multiple blocks produce the same value for the phi node (the switch condition), so that they can be folded into one block.

If we're forwarding the condition value for just a single block, that's just replacing a constant with a non-constant which seems counter-productive in general.

Looking at the example in #95919

bb3:                                              ; preds = %bb7, %bb2, %bb
 ...
 %i4 = phi i64 [ 1, %bb7 ], [ 1, %bb2 ], [ %arg1, %bb ]

We can replace [ 1, %bb2 ] with [ %arg1, %bb2 ].

What makes that beneficial is that the incoming value for %bb is already %arg1. That makes me think the right fix is rather to take that into account in FindPHIForConditionForwarding, something like:

diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
index 4e2dc7f2b2f4..980dee7707ac 100644
--- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -5743,7 +5743,7 @@ static bool eliminateDeadSwitchCases(SwitchInst *SI, DomTreeUpdater *DTU,
 /// by an unconditional branch), look at the phi node for BB in the successor
 /// block and see if the incoming value is equal to CaseValue. If so, return
 /// the phi node, and set PhiIndex to BB's index in the phi node.
-static PHINode *FindPHIForConditionForwarding(ConstantInt *CaseValue,
+static PHINode *FindPHIForConditionForwarding(Value *SwitchCondition, ConstantInt *CaseValue,
                                               BasicBlock *BB, int *PhiIndex) {
   if (BB->getFirstNonPHIOrDbg() != BB->getTerminator())
     return nullptr; // BB must be empty to be a candidate for simplification.
@@ -5761,7 +5761,7 @@ static PHINode *FindPHIForConditionForwarding(ConstantInt *CaseValue,
     assert(Idx >= 0 && "PHI has no entry for predecessor?");
 
     Value *InValue = PHI.getIncomingValue(Idx);
-    if (InValue != CaseValue)
+    if (InValue != CaseValue && InValue != SwitchCondition)
       continue;
 
     *PhiIndex = Idx;
@@ -5811,7 +5811,7 @@ static bool ForwardSwitchConditionToPHI(SwitchInst *SI) {
 
     // Collect phi nodes that are indirectly using this switch's case constants.
     int PhiIdx;
-    if (auto *Phi = FindPHIForConditionForwarding(CaseValue, CaseDest, &PhiIdx))
+    if (auto *Phi = FindPHIForConditionForwarding(SI->getCondition(), CaseValue, CaseDest, &PhiIdx))
       ForwardingNodes[Phi].push_back(PhiIdx);
   }

dianqk · 2024-06-19T12:58:05Z

Thank you for your explanation. However, I still think is meaningful.

I've chosen some examples to demonstrate the final results in assembly code. Analyzing more practical changes might be challenging, so I'm comparing the number of instructions, which might not be ideal but should at least show that this transformation is not harmful.

I replaced undef with other values, and this transformation resulted in a slight increase in instructions: https://alive2.llvm.org/ce/z/2yE6g_ , but, on arm64, the instruction count remains the same: https://alive2.llvm.org/ce/z/zMVH2_
I continued by modifying the switch conditions with other values, which reduced some instructions: https://alive2.llvm.org/ce/z/y2Xa8k (Similarly for arm64: https://alive2.llvm.org/ce/z/mfKixU)
I removed some code, which I believe is an improvement: https://alive2.llvm.org/ce/z/DfLwMV
Another similar example also shows improvement: https://alive2.llvm.org/ce/z/hwJheB

I suspect the main optimization here comes from register reuse. I'm not sure how this would perform within a large function body, as it might complicate instruction selection.

For me, another important point is that we can easily revert this forward if needed for other Passes. However, turning constants back into variables requires the dominance information.

zmodem · 2024-06-20T07:50:04Z

(I can't access the alive2 links, the server seems to be down?)

I don't have any strong opinion about whether always "forwarding" the switch condition is right or wrong. I suppose it comes down to what the following passes are best able to handle, and @nikic is probably the right person to answer that.

On the one hand, it seems bad to replace constants with a variable -- that's why 4ab4a8e was conservative and only does it when it's likely to cause blocks to get folded. On the other hand, as you say, it's easy to propagate such constants later.

I do think my diff above would fix your use case? So if we do decide to "always forward" I think we need some motivation, and we should update the comment for FindPHIForConditionForwarding with that motivation rather than talk about folding away destination blocks.

nikic · 2024-06-20T08:08:33Z

It's a tricky question. I'd generally say that having constants instead of variables is "more canonical" -- and in fact, we have quite a lot of passes that will undo what SimplifyCFG does here. At least IPSCCP, CVP and GVN do this. This is bad in itself, because it means that IR toggles back and forth between two forms. Conversely, it also means that many passes have a chance to optimize IR in both forms. The backend generally prefers phi operands not to be constants (though this is a pretty generic problem and SimplifyCFG isn't really the place to address it I think).

dianqk · 2024-06-20T08:22:56Z

(I can't access the alive2 links, the server seems to be down?)

Fixed. GitHub seems to not recognize _.

I do think my diff above would fix your use case? So if we do decide to "always forward" I think we need some motivation, and we should update the comment for FindPHIForConditionForwarding with that motivation rather than talk about folding away destination blocks.

I think so, but it would also make this part of the code a bit more complex. In fact, I don't have a clear choice for the change here.

dianqk · 2024-06-20T08:28:38Z

It's a tricky question. I'd generally say that having constants instead of variables is "more canonical" -- and in fact, we have quite a lot of passes that will undo what SimplifyCFG does here. At least IPSCCP, CVP and GVN do this. This is bad in itself, because it means that IR toggles back and forth between two forms. Conversely, it also means that many passes have a chance to optimize IR in both forms. The backend generally prefers phi operands not to be constants (though this is a pretty generic problem and SimplifyCFG isn't really the place to address it I think).

I have seen some similar things. These transforms are just back and forth, and it doesn't seem like they are being utilized.
I think a possible solution could be to treat this as a result of some analysis pass, and apply it when it's beneficial to use this result for certain transformations. Then transform it to phi operands before the backend.

dianqk · 2024-06-20T08:33:07Z

In this PR, I will change it to forward when it finds phi folding, which aligns more with SimplifyCFG behavior.

dianqk · 2024-06-20T13:17:48Z

This patch will cause values in the PHI to become switch condition values, leading to an infinite loop. Additionally, it doesn't align well with the intent of the function. I have changed it to check if the PHI already contains the switch condition value.

Forwarding is meaningful for the test case in this PR: https://alive2.llvm.org/ce/z/Z46qt6.
Based on the above discussions, I plan to implement the following changes in subsequent PRs:

Forward switch condition value to constant before backend execution
Forward only when the PHI can be folded. For instance, when there are multiple PHI nodes, checking only itself is insufficient. This should also help avoid potentially pointless transforms with IPSCCP and other passes, possibly reducing compilation time slightly.

dianqk · 2024-06-25T13:30:34Z

@zmodem Any more comments? :)

vitalybuka · 2024-06-26T22:56:31Z

https://lab.llvm.org/buildbot/#/builders/72/builds/483 is likely flake