[llvm] [AMDGPU] Search for literals among explicit operands only (PR #130771)

[Diana Picus via llvm-commits]

https://github.com/rovka updated https://github.com/llvm/llvm-project/pull/130771

>From 0b1b82d52aa120a874655c9ee901dd4016de76e9 Mon Sep 17 00:00:00 2001
From: Diana Picus <diana-magda.picus at amd.com>
Date: Tue, 11 Mar 2025 13:33:38 +0100
Subject: [PATCH 1/2] [AMDGPU] Search for literals among explicit operands only

The code in SIInstrInfo::isOperandLegal crashes in some rare cases when
dealing with call pseudos, which are variadic and may thus have more
operands than those listed in their instruction descriptions. The excess
operands will usually be implicit registers, but may also be the call
preserved reg mask.

This patch attempts to fix the issue by iterating only through the
explicit operands while searching for literal operands.
---
 llvm/lib/Target/AMDGPU/SIInstrInfo.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
index ae285d069d876..da8533fd5ef5d 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
@@ -6063,7 +6063,7 @@ bool SIInstrInfo::isOperandLegal(const MachineInstr &MI, unsigned OpIdx,
     }
   } else if (!IsInlineConst && !MO->isReg() && isSALU(MI)) {
     // There can be at most one literal operand, but it can be repeated.
-    for (unsigned i = 0, e = MI.getNumOperands(); i != e; ++i) {
+    for (unsigned i = 0, e = MI.getNumExplicitOperands(); i != e; ++i) {
       if (i == OpIdx)
         continue;
       const MachineOperand &Op = MI.getOperand(i);

>From 89e4758397bd59174fa2b5cb683b105f5ef534e4 Mon Sep 17 00:00:00 2001
From: Diana Picus <diana-magda.picus at amd.com>
Date: Tue, 11 Mar 2025 17:22:36 +0100
Subject: [PATCH 2/2] Add unit test

---
 llvm/unittests/Target/AMDGPU/CMakeLists.txt |  1 +
 llvm/unittests/Target/AMDGPU/InstrInfo.cpp  | 57 +++++++++++++++++++++
 2 files changed, 58 insertions(+)
 create mode 100644 llvm/unittests/Target/AMDGPU/InstrInfo.cpp

diff --git a/llvm/unittests/Target/AMDGPU/CMakeLists.txt b/llvm/unittests/Target/AMDGPU/CMakeLists.txt
index ca8f48bc393ef..7ad12ed556d3e 100644
--- a/llvm/unittests/Target/AMDGPU/CMakeLists.txt
+++ b/llvm/unittests/Target/AMDGPU/CMakeLists.txt
@@ -22,5 +22,6 @@ add_llvm_target_unittest(AMDGPUTests
   CSETest.cpp
   DwarfRegMappings.cpp
   ExecMayBeModifiedBeforeAnyUse.cpp
+  InstrInfo.cpp
   PALMetadata.cpp
   )
diff --git a/llvm/unittests/Target/AMDGPU/InstrInfo.cpp b/llvm/unittests/Target/AMDGPU/InstrInfo.cpp
new file mode 100644
index 0000000000000..0ea81973b1406
--- /dev/null
+++ b/llvm/unittests/Target/AMDGPU/InstrInfo.cpp
@@ -0,0 +1,57 @@
+//===- llvm/unittests/Target/AMDGPU/ExecMayBeModifiedBeforeAnyUse.cpp -----===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "AMDGPUTargetMachine.h"
+#include "AMDGPUUnitTests.h"
+#include "llvm/CodeGen/GlobalISel/MachineIRBuilder.h"
+#include "llvm/CodeGen/MachineModuleInfo.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+TEST(AMDGPU, IsOperandLegal) {
+  auto TM = createAMDGPUTargetMachine("amdgcn-amd-", "gfx1200", "");
+  if (!TM)
+    GTEST_SKIP();
+
+  GCNSubtarget ST(TM->getTargetTriple(), std::string(TM->getTargetCPU()),
+                  std::string(TM->getTargetFeatureString()), *TM);
+
+  LLVMContext Ctx;
+  Module Mod("Module", Ctx);
+  Mod.setDataLayout(TM->createDataLayout());
+
+  auto *Type = FunctionType::get(Type::getVoidTy(Ctx), false);
+  auto *F = Function::Create(Type, GlobalValue::ExternalLinkage, "Test", &Mod);
+
+  MachineModuleInfo MMI(TM.get());
+  auto MF =
+      std::make_unique<MachineFunction>(*F, *TM, ST, MMI.getContext(), 42);
+  auto *BB = MF->CreateMachineBasicBlock();
+  MF->push_back(BB);
+
+  auto E = BB->end();
+  DebugLoc DL;
+  const auto &TII = *ST.getInstrInfo();
+  const auto &TRI = *ST.getRegisterInfo();
+  auto &MRI = MF->getRegInfo();
+
+  Register VReg = MRI.createVirtualRegister(&AMDGPU::CCR_SGPR_64RegClass);
+  MachineInstr *Callee =
+      BuildMI(*BB, E, DL, TII.get(AMDGPU::S_MOV_B64), VReg).addGlobalAddress(F);
+  MachineInstr *Call =
+      BuildMI(*BB, E, DL, TII.get(AMDGPU::SI_CALL), AMDGPU::SGPR30_SGPR31)
+          .addReg(VReg)
+          .addImm(0)
+          .addRegMask(TRI.getCallPreservedMask(*MF, CallingConv::AMDGPU_Gfx))
+          .addReg(AMDGPU::VGPR0, RegState::Implicit)
+          .addReg(AMDGPU::VGPR1, RegState::Implicit);
+
+  // This shouldn't crash.
+  ASSERT_FALSE(TII.isOperandLegal(*Call, /*OpIdx=*/0, &Callee->getOperand(1)));
+}