Skip to content

[Mips] Add the missing judgment when processing function handleMFLOSlot #121463

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

Jump to bottom
Merged
merged 1 commit into from
Jan 24, 2025
Merged

[Mips] Add the missing judgment when processing function handleMFLOSlot #121463

merged 1 commit into from
Jan 24, 2025

Conversation

yingopq
Copy link
Contributor

@yingopq yingopq commented Jan 2, 2025

In function handleMFLOSlot, we may get a variable LastInstInFunction with a value of true from function getNextMachineInstr and IInSlot may be null which would trigger an assert.
So we need to skip this case.

Fix #118223.

@yingopq
Copy link
Contributor Author

yingopq commented Jan 13, 2025

@wzssyqa could you help review, thanks!

@yingopq
Copy link
Contributor Author

yingopq commented Jan 20, 2025

Ping

@wzssyqa
Copy link
Contributor

wzssyqa commented Jan 20, 2025

I think that we need a testcase for it.

wzssyqa
wzssyqa reviewed Jan 20, 2025
View reviewed changes
llvm/lib/Target/Mips/MipsBranchExpansion.cpp Outdated
@@ -751,9 +751,8 @@ bool MipsBranchExpansion::handleMFLOSlot(Pred Predicate, Safe SafeInSlot) {
for (MachineFunction::iterator FI = MFp->begin(); FI != MFp->end(); ++FI) {
for (Iter I = FI->begin(); I != FI->end(); ++I) {

if (!Predicate(*I) && !hasPendingMFLO) {
if (!Predicate(*I) && !hasPendingMFLO)
Copy link
Contributor

Choose a reason for hiding this comment

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

This change seems not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, applied.

@wzssyqa
Copy link
Contributor

wzssyqa commented Jan 20, 2025

I guess that there may be some other problem: why this patch can effects MIPSr2?
I guess that the default port of FreeBSD is r2？

@yingopq
Copy link
Contributor Author

yingopq commented Jan 20, 2025

I guess that there may be some other problem: why this patch can effects MIPSr2? I guess that the default port of FreeBSD is r2？

We have already made a restriction at the beginning.

if (STI->hasMips32() || STI->hasMips5())
    return false;

@yingopq
Copy link
Contributor Author

yingopq commented Jan 20, 2025

I guess that there may be some other problem: why this patch can effects MIPSr2? I guess that the default port of FreeBSD is r2？

In file "clang/lib/Driver/ToolChains/Arch/Mips.cpp", have the following define.

// MIPS2 is the default for mips(el)?-unknown-freebsd.
  // MIPS3 is the default for mips64(el)?-unknown-freebsd.
  if (Triple.isOSFreeBSD()) {
    DefMips32CPU = "mips2";
    DefMips64CPU = "mips3";
  }

@yingopq yingopq force-pushed the Fix_bug_issue_118223 branch 4 times, most recently from c078903 to 6de9fc2 Compare January 24, 2025 03:37
@yingopq
Copy link
Contributor Author

yingopq commented Jan 24, 2025 •
edited
Loading

I think that we need a testcase for it.

OK, added. Please help review again, thanks!

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 24, 2025 •
edited
Loading

✅ With the latest revision this PR passed the undef deprecator.

wzssyqa
wzssyqa reviewed Jan 24, 2025
View reviewed changes
llvm/test/CodeGen/Mips/llvm-ir/sdiv-freebsd.ll
@@ -0,0 +1,67 @@
; RUN: llc -filetype=asm < %s -mcpu=mips2 | FileCheck %s -check-prefixes=MIPS2
;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should keep the IR file simple.
we can use

RUN: llc -filetype=asm --mtriple=mips -mcpu=mips2 < %s | FileCheck %s -check-prefixes=MIPS2

So that we won't need

target datalayout = "E-m:m-p:32:32-i8:8:32-i16:16:32-i64:64-n32-S64"
target triple = "mips-unknown-freebsd"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, applied.

llvm/test/CodeGen/Mips/llvm-ir/sdiv-freebsd.ll Outdated
; }

target datalayout = "E-m:m-p:32:32-i8:8:32-i16:16:32-i64:64-n32-S64"
target triple = "mips-unknown-freebsd"
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove these 2 lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, applied.

llvm/test/CodeGen/Mips/llvm-ir/sdiv-freebsd.ll Outdated
@l2arc_feed_secs = local_unnamed_addr global i32 0, align 4
@l2arc_write_interval_next = local_unnamed_addr global i32 0, align 4

; Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(readwrite, argmem: none, inaccessiblemem: none)
Copy link
Contributor

Choose a reason for hiding this comment

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

This line can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, applied.

llvm/test/CodeGen/Mips/llvm-ir/sdiv-freebsd.ll Outdated
store i32 %interval.0, ptr @l2arc_write_interval_next, align 4, !tbaa !2
ret void
}

Copy link
Contributor

Choose a reason for hiding this comment

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

The following lines can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, because I refer to some other test files that generate .ll from .c, which are all included, I will delete them later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, applied.

llvm/test/CodeGen/Mips/llvm-ir/sdiv-freebsd.ll Outdated
; MIPS2-NEXT: j $BB0_3
; MIPS2-NEXT: nop
entry:
%0 = load i32, ptr @l2arc_write_interval_wrote, align 4, !tbaa !2
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove all , !tbaa !2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, applied.

wzssyqa
wzssyqa reviewed Jan 24, 2025
View reviewed changes
llvm/test/CodeGen/Mips/llvm-ir/sdiv-freebsd.ll Outdated
br label %if.end

if.end: ; preds = %if.then, %entry
%interval.0 = phi i32 [ %div, %if.then ], [ undef, %entry ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Use poison instead of undef.

llvm/test/CodeGen/Mips/llvm-ir/sdiv-freebsd.ll Outdated
; clang -cc1 -triple mips-unknown-freebsd -target-cpu mips2 -O2 -emit-llvm test.c -o test.ll
; int l2arc_feed_secs, l2arc_feed_min_ms, l2arc_write_interval_wrote, l2arc_write_interval_next;
; void l2arc_write_interval() {
; int interval;
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should use

int interval = l2arc_write_interval_next;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I initialized the variable interval to 0, and it would not produce undef.

@yingopq
[Mips] Add the missing judgment when processing function handleMFLOSlot
08a4995 
In function handleMFLOSlot, we may get a variable LastInstInFunction
with a value of true from function getNextMachineInstr and IInSlot may
be null which would trigger an assert.
So we need to skip this case.

Fix llvm#118223.
@yingopq yingopq force-pushed the Fix_bug_issue_118223 branch from 6de9fc2 to 08a4995 Compare January 24, 2025 09:54
@wzssyqa wzssyqa merged commit d6e0798 into llvm:main Jan 24, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wzssyqa wzssyqa wzssyqa left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[llvm][Mips] After 677177bb60d, Assertion failed: (!NodePtr->isKnownSentinel()), function operator*
2 participants
@yingopq @wzssyqa