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.

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

Conversation

hpoussin
Copy link
Contributor

Previously, this was calling TargetInstrInfo::getNop(), which contains:
llvm_unreachable("Not implemented");

Fixes #134913.

@llvmbot llvmbot added the clang Clang issues not falling into any other category label Apr 13, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 13, 2025

@llvm/pr-subscribers-clang

Author: Hervé Poussineau (hpoussin)

Changes

Previously, this was calling TargetInstrInfo::getNop(), which contains:
llvm_unreachable("Not implemented");

Fixes #134913.


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

3 Files Affected:

  • (added) clang/test/CodeGen/Mips/unreachable.cpp (+13)
  • (modified) llvm/lib/Target/Mips/MipsInstrInfo.cpp (+7)
  • (modified) llvm/lib/Target/Mips/MipsInstrInfo.h (+2)
diff --git a/clang/test/CodeGen/Mips/unreachable.cpp b/clang/test/CodeGen/Mips/unreachable.cpp
new file mode 100644
index 0000000000000..04fec8598088d
--- /dev/null
+++ b/clang/test/CodeGen/Mips/unreachable.cpp
@@ -0,0 +1,13 @@
+// REQUIRES: mips-registered-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();
+}
diff --git a/llvm/lib/Target/Mips/MipsInstrInfo.cpp b/llvm/lib/Target/Mips/MipsInstrInfo.cpp
index b81bb1186de72..0abe228b7547a 100644
--- a/llvm/lib/Target/Mips/MipsInstrInfo.cpp
+++ b/llvm/lib/Target/Mips/MipsInstrInfo.cpp
@@ -87,6 +87,13 @@ 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>"
+  Nop.setOpcode(Mips::SSNOP);
+  return Nop;
+}
+
 //===----------------------------------------------------------------------===//
 // Branch Analysis
 //===----------------------------------------------------------------------===//
diff --git a/llvm/lib/Target/Mips/MipsInstrInfo.h b/llvm/lib/Target/Mips/MipsInstrInfo.h
index 06964c0161b4b..367f0ac5e8390 100644
--- a/llvm/lib/Target/Mips/MipsInstrInfo.h
+++ b/llvm/lib/Target/Mips/MipsInstrInfo.h
@@ -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,

Copy link

github-actions bot commented Apr 13, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

Previously, this was calling TargetInstrInfo::getNop(),
which contains:
 llvm_unreachable("Not implemented");

Fixes llvm#134913.
@brad0
Copy link
Contributor

brad0 commented Apr 17, 2025

cc @MaskRay

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.

@@ -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=

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[MIPS] Compiler crash when using __builtin_unreachable
5 participants