[PowerPC] need to set CallFrameSize for the pass PPCReduceCRLogicals when insert a new block #151017

diggerlin · 2025-07-28T18:38:28Z

In the [CodeGen] Store call frame size in MachineBasicBlock, it mentions When a basic block has been split in the middle of a call sequence. the call frame size may not be zero, it need to set the setCallFrameSize for the new MachineBasicBlock. but in the function splitMBB(BlockSplitInfo &BSI) in the llvm/lib/Target/PowerPC/PPCReduceCRLogicals.cpp , it do not setCallFrameSzie for the new MachineBasicBlock NewMBB, we will setCallFrameSzie in the patch.

the patch fix the crash mention in #144594 (comment)

llvmbot · 2025-07-28T18:38:58Z

@llvm/pr-subscribers-backend-powerpc

Author: zhijian lin (diggerlin)

Changes

In the https://reviews.llvm.org/D156113 [CodeGen] Store call frame size in MachineBasicBlock, When a basic block has been split in the middle of a call sequence. the call frame size may not be zero, it need to set the setCallFrameSize for the new MachineBasicBlock. but in the function splitMBB(BlockSplitInfo &BSI) in the llvm/lib/Target/PowerPC/PPCReduceCRLogicals.cpp , it do not setCallFrameSzie for the new MachineBasicBlock NewMBB, we will setCallFrameSzie in the patch.

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

2 Files Affected:

(modified) llvm/lib/Target/PowerPC/PPCReduceCRLogicals.cpp (+5)
(added) llvm/test/CodeGen/PowerPC/ppc_reduce_cr_logicals.ll (+75)

diff --git a/llvm/lib/Target/PowerPC/PPCReduceCRLogicals.cpp b/llvm/lib/Target/PowerPC/PPCReduceCRLogicals.cpp
index 0ffd35dfa279c..f83b2fb8e65a2 100644
--- a/llvm/lib/Target/PowerPC/PPCReduceCRLogicals.cpp
+++ b/llvm/lib/Target/PowerPC/PPCReduceCRLogicals.cpp
@@ -248,6 +248,11 @@ static bool splitMBB(BlockSplitInfo &BSI) {
   }
   addIncomingValuesToPHIs(NewBRTarget, ThisMBB, NewMBB, MRI);
 
+  // Set the call frame size on ThisMBB to the new basic blocks.
+  // See https://reviews.llvm.org/D156113.
+  unsigned CallFrameSize = TII->getCallFrameSizeAt(ThisMBB->back());
+  NewMBB->setCallFrameSize(CallFrameSize);
+
   LLVM_DEBUG(dbgs() << "After splitting, ThisMBB:\n"; ThisMBB->dump());
   LLVM_DEBUG(dbgs() << "NewMBB:\n"; NewMBB->dump());
   LLVM_DEBUG(dbgs() << "New branch-to block:\n"; NewBRTarget->dump());
diff --git a/llvm/test/CodeGen/PowerPC/ppc_reduce_cr_logicals.ll b/llvm/test/CodeGen/PowerPC/ppc_reduce_cr_logicals.ll
new file mode 100644
index 0000000000000..7502991a58cc4
--- /dev/null
+++ b/llvm/test/CodeGen/PowerPC/ppc_reduce_cr_logicals.ll
@@ -0,0 +1,75 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-gnu-linux  < %s | FileCheck %s -check-prefix=CHECK
+
+define ptr @xe_migrate_copy(i1 %tobool, i1 %tobool6) {
+; CHECK-LABEL: xe_migrate_copy:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    mfcr 12
+; CHECK-NEXT:    stw 12, 8(1)
+; CHECK-NEXT:    mflr 0
+; CHECK-NEXT:    stdu 1, -176(1)
+; CHECK-NEXT:    std 0, 192(1)
+; CHECK-NEXT:    .cfi_def_cfa_offset 176
+; CHECK-NEXT:    .cfi_offset lr, 16
+; CHECK-NEXT:    .cfi_offset r27, -40
+; CHECK-NEXT:    .cfi_offset r28, -32
+; CHECK-NEXT:    .cfi_offset r29, -24
+; CHECK-NEXT:    .cfi_offset r30, -16
+; CHECK-NEXT:    .cfi_offset cr2, 8
+; CHECK-NEXT:    std 27, 136(1) # 8-byte Folded Spill
+; CHECK-NEXT:    andi. 4, 4, 1
+; CHECK-NEXT:    crmove 8, 1
+; CHECK-NEXT:    andi. 3, 3, 1
+; CHECK-NEXT:    std 28, 144(1) # 8-byte Folded Spill
+; CHECK-NEXT:    crmove 9, 1
+; CHECK-NEXT:    std 29, 152(1) # 8-byte Folded Spill
+; CHECK-NEXT:    std 30, 160(1) # 8-byte Folded Spill
+; CHECK-NEXT:    lwz 30, 132(1)
+; CHECK-NEXT:    ld 28, 8(0)
+; CHECK-NEXT:    ld 29, 16(0)
+; CHECK-NEXT:    ld 27, 0(0)
+; CHECK-NEXT:    std 2, 40(1)
+; CHECK-NEXT:    b .LBB0_3
+; CHECK-NEXT:  .LBB0_1: # %if.then36
+; CHECK-NEXT:    #
+; CHECK-NEXT:    li 6, 1
+; CHECK-NEXT:  .LBB0_2: # %if.then36
+; CHECK-NEXT:    #
+; CHECK-NEXT:    mtctr 27
+; CHECK-NEXT:    li 4, 0
+; CHECK-NEXT:    li 5, 0
+; CHECK-NEXT:    li 7, 0
+; CHECK-NEXT:    li 8, 0
+; CHECK-NEXT:    mr 9, 30
+; CHECK-NEXT:    li 10, 0
+; CHECK-NEXT:    mr 2, 28
+; CHECK-NEXT:    mr 11, 29
+; CHECK-NEXT:    bctrl
+; CHECK-NEXT:    ld 2, 40(1)
+; CHECK-NEXT:  .LBB0_3: # %if.then36
+; CHECK-NEXT:    #
+; CHECK-NEXT:    lwz 3, 0(0)
+; CHECK-NEXT:    cmplwi 3, 0
+; CHECK-NEXT:    li 3, 0
+; CHECK-NEXT:    crandc 20, 8, 2
+; CHECK-NEXT:    std 3, 112(1)
+; CHECK-NEXT:    bc 12, 20, .LBB0_1
+; CHECK-NEXT:  # %bb.4: # %if.then36
+; CHECK-NEXT:    #
+; CHECK-NEXT:    crand 20, 2, 9
+; CHECK-NEXT:    li 6, 0
+; CHECK-NEXT:    bc 4, 20, .LBB0_2
+; CHECK-NEXT:    b .LBB0_1
+entry:
+  %src_L0 = alloca i64, align 8
+  br label %if.then36
+
+if.then36:                                        ; preds = %if.then36, %entry
+  %0 = load i32, ptr null, align 4
+  %tobool37.not = icmp eq i32 %0, 0
+  %tobool.tobool6 = select i1 %tobool37.not, i1 %tobool, i1 %tobool6
+  %1 = load i64, ptr %src_L0, align 8
+  %conv55 = trunc i64 %1 to i32
+  %call57 = call i32 null(ptr null, ptr null, i64 0, i1 %tobool.tobool6, i64 0, i1 false, i32 %conv55, i64 0, i1 false)
+  br label %if.then36
+}

lei137 · 2025-07-28T18:59:39Z

llvm/lib/Target/PowerPC/PPCReduceCRLogicals.cpp

+  unsigned CallFrameSize = TII->getCallFrameSizeAt(ThisMBB->back());
+  NewMBB->setCallFrameSize(CallFrameSize);


nit: can we just inline this to remove the tmp variable?

lei137 · 2025-07-28T19:00:44Z

llvm/test/CodeGen/PowerPC/ppc_reduce_cr_logicals.ll

@@ -0,0 +1,75 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-gnu-linux  < %s | FileCheck %s -check-prefix=CHECK


just wondering what this lit test produces if we don't re-calculate the frame size? Does it crash?

add run line to test 32bit BE?

just wondering what this lit test produces if we don't re-calculate the frame size? Does it crash?

yes, if we don't re-calculate the frame size it crash in this test case.

lei137

LGTM.
Thx

arsenm · 2025-07-30T02:44:49Z

llvm/test/CodeGen/PowerPC/ppc_reduce_cr_logicals.ll

+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-gnu-linux  < %s | FileCheck %s -check-prefix=CHECK
+; RUN: llc -verify-machineinstrs -mtriple=powerpc-unknown-gnu-linux  < %s | FileCheck %s -check-prefix=CHECKBE
+
+define ptr @xe_migrate_copy(i1 %tobool, i1 %tobool6) {


Suggested change

define ptr @xe_migrate_copy(i1 %tobool, i1 %tobool6) {

define ptr @xe_migrate_copy(i1 %tobool, i1 %tobool6) nounwind {

arsenm · 2025-07-30T02:45:00Z

llvm/test/CodeGen/PowerPC/ppc_reduce_cr_logicals.ll

@@ -0,0 +1,124 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-gnu-linux  < %s | FileCheck %s -check-prefix=CHECK


Suggested change

; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-gnu-linux < %s | FileCheck %s -check-prefix=CHECK

; RUN: llc -mtriple=powerpc64-unknown-gnu-linux < %s | FileCheck %s -check-prefix=CHECK

Why do you feel we shouldn't test with -verify-machineinstrs? Normally I ask people to add that....

arsenm · 2025-07-30T02:45:09Z

llvm/test/CodeGen/PowerPC/ppc_reduce_cr_logicals.ll

+  br label %if.then36
+
+if.then36:                                        ; preds = %if.then36, %entry
+  %0 = load i32, ptr null, align 4


Avoid the UB null load

arsenm · 2025-07-30T02:45:15Z

llvm/test/CodeGen/PowerPC/ppc_reduce_cr_logicals.ll

+  %tobool.tobool6 = select i1 %tobool37.not, i1 %tobool, i1 %tobool6
+  %1 = load i64, ptr %src_L0, align 8
+  %conv55 = trunc i64 %1 to i32
+  %call57 = call i32 null(ptr null, ptr null, i64 0, i1 %tobool.tobool6, i64 0, i1 false, i32 %conv55, i64 0, i1 false)


Avoid UB call to null

arsenm · 2025-07-30T02:45:19Z

llvm/test/CodeGen/PowerPC/ppc_reduce_cr_logicals.ll

+  %0 = load i32, ptr null, align 4
+  %tobool37.not = icmp eq i32 %0, 0
+  %tobool.tobool6 = select i1 %tobool37.not, i1 %tobool, i1 %tobool6
+  %1 = load i64, ptr %src_L0, align 8


Use named values in tests

diggerlin · 2025-07-30T20:15:14Z

the test case llvm/test/CodeGen/PowerPC/ppc_reduce_cr_logicals.ll is generated by the llvm-reduce from #144594 (comment). the test case is crashed without the patch.

… when insert a new block

llvmbot added the backend:PowerPC label Jul 28, 2025

diggerlin requested review from lei137, nathanchance and RolandF77 July 28, 2025 18:39

diggerlin mentioned this pull request Jul 28, 2025

[Reland] [PowerPC] frontend get target feature from backend with cpu name #144594

Merged

diggerlin requested a review from daltenty July 28, 2025 18:46

lei137 reviewed Jul 28, 2025

View reviewed changes

diggerlin requested review from lei137 and arsenm July 28, 2025 19:42

lei137 approved these changes Jul 29, 2025

View reviewed changes

arsenm reviewed Jul 30, 2025

View reviewed changes

diggerlin added 5 commits July 30, 2025 20:22

[PowerPC] need to set CallFrameSize for the pass PPCReduceCRLogicals…

cf2633d

… when insert a new block

address comment

d53afd8

add 32-bit powerpc be test scenario

f4edf6f

address comment

248fb8b

get rid of ptr noundef null in test case

30258cb

diggerlin force-pushed the digger/fix-PPCReduceCRLogicals branch from 81376a1 to 30258cb Compare July 30, 2025 20:52

Merge branch 'main' into digger/fix-PPCReduceCRLogicals

73c9907

diggerlin requested a review from arsenm July 30, 2025 22:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[PowerPC] need to set CallFrameSize for the pass PPCReduceCRLogicals when insert a new block #151017

[PowerPC] need to set CallFrameSize for the pass PPCReduceCRLogicals when insert a new block #151017

diggerlin commented Jul 28, 2025 •

edited

Loading

Uh oh!

llvmbot commented Jul 28, 2025

Uh oh!

lei137 Jul 28, 2025

Uh oh!

lei137 Jul 28, 2025

Uh oh!

lei137 Jul 28, 2025

Uh oh!

diggerlin Jul 28, 2025 •

edited

Loading

Uh oh!

lei137 left a comment

Uh oh!

arsenm Jul 30, 2025

Uh oh!

arsenm Jul 30, 2025

Uh oh!

lei137 Jul 30, 2025

Uh oh!

arsenm Jul 30, 2025

Uh oh!

arsenm Jul 30, 2025

Uh oh!

arsenm Jul 30, 2025

Uh oh!

diggerlin commented Jul 30, 2025 •

edited

Loading

Uh oh!

Uh oh!

		unsigned CallFrameSize = TII->getCallFrameSizeAt(ThisMBB->back());
		NewMBB->setCallFrameSize(CallFrameSize);

		@@ -0,0 +1,75 @@
		; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
		; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-gnu-linux < %s \| FileCheck %s -check-prefix=CHECK

	define ptr @xe_migrate_copy(i1 %tobool, i1 %tobool6) {
	define ptr @xe_migrate_copy(i1 %tobool, i1 %tobool6) nounwind {

		@@ -0,0 +1,124 @@
		; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
		; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-gnu-linux < %s \| FileCheck %s -check-prefix=CHECK

	; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-gnu-linux < %s \| FileCheck %s -check-prefix=CHECK
	; RUN: llc -mtriple=powerpc64-unknown-gnu-linux < %s \| FileCheck %s -check-prefix=CHECK

[PowerPC] need to set CallFrameSize for the pass PPCReduceCRLogicals when insert a new block #151017

Are you sure you want to change the base?

[PowerPC] need to set CallFrameSize for the pass PPCReduceCRLogicals when insert a new block #151017

Conversation

diggerlin commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diggerlin Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lei137 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diggerlin commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

diggerlin commented Jul 28, 2025 •

edited

Loading

diggerlin Jul 28, 2025 •

edited

Loading

diggerlin commented Jul 30, 2025 •

edited

Loading