Skip to content

[Mips] Implement MipsInstrInfo::getNop() operation #135524

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 1 commit into
base: main
Choose a base branch
from
Open

[Mips] Implement MipsInstrInfo::getNop() operation #135524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 13 additions & 0 deletions clang/test/CodeGen/Mips/unreachable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// REQUIRES: mips-registered-target
Copy link
Member

Choose a reason for hiding this comment

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

https://maskray.me/blog/2021-08-08-toolchain-testing#the-test-checks-too-little#the-test-checks-at-the-wrong-layer

Should generally avoid clang/test/CodeGen test for lib/Target changes. llvm/test/MC/Mips testing is in a pretty bad state. You might want to check RISCV/LoongArch.

(I will have limited internet access between April 20th and May 4th, and my response time may be delayed..)

Copy link
Member

Choose a reason for hiding this comment

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

clang++ -target "mipsel-w64-windows-gnu" -c test.cpp avoid deprecated -target . Use --target=

// RUN: %clang_cc1 -triple mipsel-w64-windows-gnu -x c++ -mrelocation-model static -emit-obj %s -o - | llvm-objdump -a - | FileCheck %s
// CHECK: file format coff-mips

[[__noreturn__]] inline void g() {
__builtin_unreachable();
}

void f(int i)
{
if (i == 0)
g();
}
8 changes: 8 additions & 0 deletions llvm/lib/Target/Mips/MipsInstrInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,14 @@ MipsInstrInfo::GetMemOperand(MachineBasicBlock &MBB, int FI,
MFI.getObjectAlign(FI));
}

MCInst MipsInstrInfo::getNop() const {
MCInst Nop;
// using Mips::NOP gives
// "fatal error: error in backend: Not supported instr: <MCInst 580>"
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we need address this error and use Mips::NOP?
Mips::SSNOP is deprecated on MIPS32r6/MIPS64r6.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This patch is already an improvement, because it replaces a compiler crash by the use of a valid instruction (although deprecated in recent architectures).
However, you're right that it will be better to address this error and use Mips::NOP. Unfortunately, I think I won't be able to do it myself.

Nop.setOpcode(Mips::SSNOP);
return Nop;
}

//===----------------------------------------------------------------------===//
// Branch Analysis
//===----------------------------------------------------------------------===//
Expand Down
2 changes: 2 additions & 0 deletions llvm/lib/Target/Mips/MipsInstrInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ class MipsInstrInfo : public MipsGenInstrInfo {

static const MipsInstrInfo *create(MipsSubtarget &STI);

MCInst getNop() const override;

/// Branch Analysis
bool analyzeBranch(MachineBasicBlock &MBB, MachineBasicBlock *&TBB,
MachineBasicBlock *&FBB,
Expand Down