[llvm] [Mips] Use a Target ISD opcode for PseudoD_SELECT (PR #84294)

Roger Ferrer Ibáñez via llvm-commits llvm-commits at lists.llvm.org
Thu Mar 7 01:42:10 PST 2024


https://github.com/rofirrim created https://github.com/llvm/llvm-project/pull/84294

The Mips target uses two TargetOpcode enumerators called `PseudoD_SELECT_I` and `PseudoD_SELECT_I64`. A SDAG node is created using these enumerators which is manually selected in `MipsSEISelDAGToDAG.cpp` and ultimately expanded in `EmitInstrWithCustomInserter` in `MipsISelLowering.cpp`.

This is not causing any upstream build to fail at the moment but it is not guaranteed that these enumerators do not clash with Target ISD nodes (i.e. those in the `MipsISD` namespace). We have seen this happening in our downstream builds in which `Mips::PseudoD_SELECT_I` ends having the same integer value as `MipsISD::VEXTRACT_ZEXT_ELT`. This confuses the function `trySelect` in `MipsSEISelDAGToDAG.cpp` and causes a crash in 3 tests.

This change adds a new Target ISD opcode for these two cases and uses them for the SDAG nodes. No test is included because this is a potential error in the future not one that can be demonstrated in the current codebase.

>From 5468b0a8c47ec19177c8e11ddc6eee13f4cc3bfc Mon Sep 17 00:00:00 2001
From: Roger Ferrer Ibanez <roger.ferrer at bsc.es>
Date: Wed, 6 Mar 2024 18:59:47 +0000
Subject: [PATCH] [Mips] Use a Target ISD opcode for PseudoD_SELECT

The Mips target uses two TargetOpcode enumerators called
`PseudoD_SELECT_I` and `PseudoD_SELECT_I64`. A SDAG node is created
using these enumerators which is manually selected in
`MipsSEISelDAGToDAG.cpp` and ultimately expanded in
`EmitInstrWithCustomInserter` in `MipsISelLowering.cpp`.

This is not causing any upstream build to fail at the moment but it is
not guaranteed that these enumerators do not clash with Target ISD nodes
(i.e. those in the `MipsISD` namespace). We have seen this happening in our
downstream builds in which `Mips::PseudoD_SELECT_I` ends having the same
integer value as `MipsISD::VEXTRACT_ZEXT_ELT`. This confuses the function
`trySelect` in `MipsSEISelDAGToDAG.cpp` and causes a crash in 3 tests.

This change adds a new Target ISD opcode for these two cases and uses
them for the SDAG nodes. No test is included because this is a potential
error in the future not one that can be demonstrated in the current
codebase.
---
 llvm/lib/Target/Mips/MipsISelLowering.cpp   | 6 ++++--
 llvm/lib/Target/Mips/MipsISelLowering.h     | 4 ++++
 llvm/lib/Target/Mips/MipsSEISelDAGToDAG.cpp | 4 ++--
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/llvm/lib/Target/Mips/MipsISelLowering.cpp b/llvm/lib/Target/Mips/MipsISelLowering.cpp
index 97e830cec27cad..7e5f148e7bf4e7 100644
--- a/llvm/lib/Target/Mips/MipsISelLowering.cpp
+++ b/llvm/lib/Target/Mips/MipsISelLowering.cpp
@@ -239,6 +239,8 @@ const char *MipsTargetLowering::getTargetNodeName(unsigned Opcode) const {
   case MipsISD::MAQ_S_W_PHR:       return "MipsISD::MAQ_S_W_PHR";
   case MipsISD::MAQ_SA_W_PHL:      return "MipsISD::MAQ_SA_W_PHL";
   case MipsISD::MAQ_SA_W_PHR:      return "MipsISD::MAQ_SA_W_PHR";
+  case MipsISD::DOUBLE_SELECT_I:   return "MipsISD::DOUBLE_SELECT_I";
+  case MipsISD::DOUBLE_SELECT_I64: return "MipsISD::DOUBLE_SELECT_I64";
   case MipsISD::DPAU_H_QBL:        return "MipsISD::DPAU_H_QBL";
   case MipsISD::DPAU_H_QBR:        return "MipsISD::DPAU_H_QBR";
   case MipsISD::DPSU_H_QBL:        return "MipsISD::DPSU_H_QBL";
@@ -2652,8 +2654,8 @@ SDValue MipsTargetLowering::lowerShiftRightParts(SDValue Op, SelectionDAG &DAG,
 
   if (!(Subtarget.hasMips4() || Subtarget.hasMips32())) {
     SDVTList VTList = DAG.getVTList(VT, VT);
-    return DAG.getNode(Subtarget.isGP64bit() ? Mips::PseudoD_SELECT_I64
-                                             : Mips::PseudoD_SELECT_I,
+    return DAG.getNode(Subtarget.isGP64bit() ? MipsISD::DOUBLE_SELECT_I64
+                                             : MipsISD::DOUBLE_SELECT_I,
                        DL, VTList, Cond, ShiftRightHi,
                        IsSRA ? Ext : DAG.getConstant(0, DL, VT), Or,
                        ShiftRightHi);
diff --git a/llvm/lib/Target/Mips/MipsISelLowering.h b/llvm/lib/Target/Mips/MipsISelLowering.h
index 7d243eeca5c6c7..84ad40d6bbbe26 100644
--- a/llvm/lib/Target/Mips/MipsISelLowering.h
+++ b/llvm/lib/Target/Mips/MipsISelLowering.h
@@ -242,6 +242,10 @@ class TargetRegisterClass;
       VEXTRACT_SEXT_ELT,
       VEXTRACT_ZEXT_ELT,
 
+      // Double select nodes for machines without conditional-move.
+      DOUBLE_SELECT_I,
+      DOUBLE_SELECT_I64,
+
       // Load/Store Left/Right nodes.
       LWL = ISD::FIRST_TARGET_MEMORY_OPCODE,
       LWR,
diff --git a/llvm/lib/Target/Mips/MipsSEISelDAGToDAG.cpp b/llvm/lib/Target/Mips/MipsSEISelDAGToDAG.cpp
index c0e978018919e0..ab39d1b661ef9a 100644
--- a/llvm/lib/Target/Mips/MipsSEISelDAGToDAG.cpp
+++ b/llvm/lib/Target/Mips/MipsSEISelDAGToDAG.cpp
@@ -741,8 +741,8 @@ bool MipsSEDAGToDAGISel::trySelect(SDNode *Node) {
   switch(Opcode) {
   default: break;
 
-  case Mips::PseudoD_SELECT_I:
-  case Mips::PseudoD_SELECT_I64: {
+  case MipsISD::DOUBLE_SELECT_I:
+  case MipsISD::DOUBLE_SELECT_I64: {
     MVT VT = Subtarget->isGP64bit() ? MVT::i64 : MVT::i32;
     SDValue cond = Node->getOperand(0);
     SDValue Hi1 = Node->getOperand(1);



More information about the llvm-commits mailing list