Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cf2633d
[PowerPC] need to set CallFrameSize for the pass PPCReduceCRLogicals…
diggerlin Jul 28, 2025
d53afd8
address comment
diggerlin Jul 28, 2025
f4edf6f
add 32-bit powerpc be test scenario
diggerlin Jul 28, 2025
248fb8b
address comment
diggerlin Jul 30, 2025
30258cb
get rid of `ptr noundef null` in test case
diggerlin Jul 30, 2025
73c9907
Merge branch 'main' into digger/fix-PPCReduceCRLogicals
diggerlin Jul 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions llvm/lib/Target/PowerPC/PPCReduceCRLogicals.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,10 @@ 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.
NewMBB->setCallFrameSize(TII->getCallFrameSizeAt(ThisMBB->back()));

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());
Expand Down
88 changes: 88 additions & 0 deletions llvm/test/CodeGen/PowerPC/ppc_reduce_cr_logicals.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
; 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add run line to test 32bit BE?

Copy link
Contributor Author

@diggerlin diggerlin Jul 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it it brutally expensive and it's test noise. EXPENSIVE_CHECKS builds already run every test with the default, we shouldn't need to manually add it to every test. It should only be added in select tests that are specifically testing verification issues that appeared

Copy link
Contributor Author

@diggerlin diggerlin Aug 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-verify-machineinstrs runs MachineVerifier after a pass only if that pass modifies the MachineFunction.it can find the regression earlier if there is, the EXPENSIVE_CHECKS check different scope, I looked into the code, it looks the checking of EXPENSIVE_CHECKS can not cover the -verify-machineinstrs

if you strong suggest not to use the -verify-machineinstrs for all the test cases, I can remove the -verify-machineinstrs .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These do not have different behavior, EXPENSIVE_CHECKS just changes the default to on

We should be actively purging -verify-machineinstrs from most codegen tests. It should not be the default to add it

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

define i32 @xe_migrate_copy(ptr %m, ptr %dst, ptr %tile, ptr %0, ptr %primary_gt, i1 %tobool4, i1 %tobool9, i64 %1, i32 %conv55, i1 %tobool37.not) nounwind {
; CHECK-LABEL: xe_migrate_copy:
; CHECK: # %bb.0: # %entry
; CHECK-NEXT: mflr 0
; CHECK-NEXT: stdu 1, -128(1)
; CHECK-NEXT: lbz 4, 255(1)
; CHECK-NEXT: andi. 4, 4, 1
; CHECK-NEXT: std 0, 144(1)
; CHECK-NEXT: crmove 20, 1
; CHECK-NEXT: andi. 4, 9, 1
; CHECK-NEXT: lwz 9, 244(1)
; CHECK-NEXT: crmove 21, 1
; CHECK-NEXT: andi. 4, 8, 1
; CHECK-NEXT: li 4, 0
; CHECK-NEXT: std 4, 112(1)
; CHECK-NEXT: crandc 21, 21, 20
; CHECK-NEXT: bc 12, 21, .LBB0_2
; CHECK-NEXT: # %bb.1: # %while.body
; CHECK-NEXT: crand 20, 20, 1
; CHECK-NEXT: li 8, 0
; CHECK-NEXT: bc 4, 20, .LBB0_3
; CHECK-NEXT: .LBB0_2: # %while.body
; CHECK-NEXT: li 8, 1
; CHECK-NEXT: .LBB0_3: # %while.body
; CHECK-NEXT: li 5, 0
; CHECK-NEXT: li 6, 0
; CHECK-NEXT: mr 4, 3
; CHECK-NEXT: li 7, 0
; CHECK-NEXT: li 10, 0
; CHECK-NEXT: bl xe_migrate_ccs_copy
; CHECK-NEXT: nop
; CHECK-NEXT: addi 1, 1, 128
; CHECK-NEXT: ld 0, 16(1)
; CHECK-NEXT: mtlr 0
; CHECK-NEXT: blr
;
; CHECKBE-LABEL: xe_migrate_copy:
; CHECKBE: # %bb.0: # %entry
; CHECKBE-NEXT: mflr 0
; CHECKBE-NEXT: stwu 1, -32(1)
; CHECKBE-NEXT: lbz 4, 55(1)
; CHECKBE-NEXT: li 5, 0
; CHECKBE-NEXT: stw 0, 36(1)
; CHECKBE-NEXT: andi. 4, 4, 1
; CHECKBE-NEXT: crmove 20, 1
; CHECKBE-NEXT: andi. 4, 9, 1
; CHECKBE-NEXT: crmove 21, 1
; CHECKBE-NEXT: andi. 4, 8, 1
; CHECKBE-NEXT: lwz 4, 48(1)
; CHECKBE-NEXT: crandc 21, 21, 20
; CHECKBE-NEXT: stw 5, 24(1)
; CHECKBE-NEXT: stw 5, 20(1)
; CHECKBE-NEXT: stw 5, 16(1)
; CHECKBE-NEXT: stw 4, 12(1)
; CHECKBE-NEXT: bc 12, 21, .LBB0_2
; CHECKBE-NEXT: # %bb.1: # %while.body
; CHECKBE-NEXT: crand 20, 20, 1
; CHECKBE-NEXT: li 8, 0
; CHECKBE-NEXT: bc 4, 20, .LBB0_3
; CHECKBE-NEXT: .LBB0_2: # %while.body
; CHECKBE-NEXT: li 8, 1
; CHECKBE-NEXT: .LBB0_3: # %while.body
; CHECKBE-NEXT: mr 4, 3
; CHECKBE-NEXT: li 6, 0
; CHECKBE-NEXT: li 7, 0
; CHECKBE-NEXT: li 9, 0
; CHECKBE-NEXT: li 10, 0
; CHECKBE-NEXT: stw 8, 8(1)
; CHECKBE-NEXT: bl xe_migrate_ccs_copy
; CHECKBE-NEXT: lwz 0, 36(1)
; CHECKBE-NEXT: addi 1, 1, 32
; CHECKBE-NEXT: mtlr 0
; CHECKBE-NEXT: blr

entry:
br label %while.body

while.body:
%cond53.in = select i1 %tobool37.not, i1 %tobool4, i1 %tobool9
%call57 = call zeroext i32 @xe_migrate_ccs_copy(ptr noundef %m, ptr noundef %m, i64 0, i1 false, i64 0, i1 %cond53.in, i32 %conv55, i64 0, i1 false)
ret i32 %call57
}

declare i32 @xe_migrate_ccs_copy(ptr, ptr, i64, i1, i64, i1, i32, i64, i1)