TargetInstrInfo: make getOperandLatency return optional (NFC) #73769

artagnon · 2023-11-29T09:14:36Z

getOperandLatency has the following behavior: it returns -1 as a special value, negative numbers other than -1 on some target-specific overrides, or a valid non-negative latency. This behavior can be surprising, as some callers do arithmetic on these negative values. Change the interface of getOperandLatency to return a std::optional<unsigned> to prevent surprises in callers. While at it, change the interface of getInstrLatency to return unsigned instead of int.

This change was inspired by a refactoring in TargetSchedModel::computeOperandLatency.

fpetrogalli

Thank you for the patch.

Just some minor nits, plus a couple of comment about some follow up (or prep) work that I think would improve the TII method for retrieving the latency of an operand . Consider the TII interface chance as "nice to have", I will not hold the patch on it.

Francesco

llvm/lib/CodeGen/TargetSchedule.cpp

github-actions · 2023-11-30T09:54:03Z

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

llvmbot · 2023-11-30T15:36:09Z

@llvm/pr-subscribers-mc

@llvm/pr-subscribers-llvm-selectiondag

Author: Ramkumar Ramachandra (artagnon)

Changes

Factor out DefaultDefLatency and InstrLatency in computeOperandLatency.

Patch is 40.82 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/73769.diff

13 Files Affected:

(modified) llvm/include/llvm/CodeGen/TargetInstrInfo.h (+9-9)
(modified) llvm/include/llvm/MC/MCInstrItineraries.h (+18-15)
(modified) llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp (+5-4)
(modified) llvm/lib/CodeGen/TargetInstrInfo.cpp (+11-12)
(modified) llvm/lib/CodeGen/TargetSchedule.cpp (+12-17)
(modified) llvm/lib/MC/MCDisassembler/Disassembler.cpp (+6-5)
(modified) llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp (+58-59)
(modified) llvm/lib/Target/ARM/ARMBaseInstrInfo.h (+39-37)
(modified) llvm/lib/Target/Hexagon/HexagonInstrInfo.cpp (+6-8)
(modified) llvm/lib/Target/Hexagon/HexagonInstrInfo.h (+5-4)
(modified) llvm/lib/Target/Hexagon/HexagonSubtarget.cpp (+14-13)
(modified) llvm/lib/Target/PowerPC/PPCInstrInfo.cpp (+11-12)
(modified) llvm/lib/Target/PowerPC/PPCInstrInfo.h (+9-7)

diff --git a/llvm/include/llvm/CodeGen/TargetInstrInfo.h b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
index de065849eaa6ebc..fbff66f4ce544ef 100644
--- a/llvm/include/llvm/CodeGen/TargetInstrInfo.h
+++ b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
@@ -1705,9 +1705,9 @@ class TargetInstrInfo : public MCInstrInfo {
     return Opcode <= TargetOpcode::COPY;
   }
 
-  virtual int getOperandLatency(const InstrItineraryData *ItinData,
-                                SDNode *DefNode, unsigned DefIdx,
-                                SDNode *UseNode, unsigned UseIdx) const;
+  virtual std::optional<unsigned>
+  getOperandLatency(const InstrItineraryData *ItinData, SDNode *DefNode,
+                    unsigned DefIdx, SDNode *UseNode, unsigned UseIdx) const;
 
   /// Compute and return the use operand latency of a given pair of def and use.
   /// In most cases, the static scheduling itinerary was enough to determine the
@@ -1717,10 +1717,10 @@ class TargetInstrInfo : public MCInstrInfo {
   /// This is a raw interface to the itinerary that may be directly overridden
   /// by a target. Use computeOperandLatency to get the best estimate of
   /// latency.
-  virtual int getOperandLatency(const InstrItineraryData *ItinData,
-                                const MachineInstr &DefMI, unsigned DefIdx,
-                                const MachineInstr &UseMI,
-                                unsigned UseIdx) const;
+  virtual std::optional<unsigned>
+  getOperandLatency(const InstrItineraryData *ItinData,
+                    const MachineInstr &DefMI, unsigned DefIdx,
+                    const MachineInstr &UseMI, unsigned UseIdx) const;
 
   /// Compute the instruction latency of a given instruction.
   /// If the instruction has higher cost when predicated, it's returned via
@@ -1731,8 +1731,8 @@ class TargetInstrInfo : public MCInstrInfo {
 
   virtual unsigned getPredicationCost(const MachineInstr &MI) const;
 
-  virtual int getInstrLatency(const InstrItineraryData *ItinData,
-                              SDNode *Node) const;
+  virtual unsigned getInstrLatency(const InstrItineraryData *ItinData,
+                                   SDNode *Node) const;
 
   /// Return the default expected latency for a def based on its opcode.
   unsigned defaultDefLatency(const MCSchedModel &SchedModel,
diff --git a/llvm/include/llvm/MC/MCInstrItineraries.h b/llvm/include/llvm/MC/MCInstrItineraries.h
index 652922feddc3384..adf2ae7df2b6215 100644
--- a/llvm/include/llvm/MC/MCInstrItineraries.h
+++ b/llvm/include/llvm/MC/MCInstrItineraries.h
@@ -17,6 +17,7 @@
 
 #include "llvm/MC/MCSchedule.h"
 #include <algorithm>
+#include <optional>
 
 namespace llvm {
 
@@ -164,16 +165,17 @@ class InstrItineraryData {
 
   /// Return the cycle for the given class and operand.  Return -1 if no
   /// cycle is specified for the operand.
-  int getOperandCycle(unsigned ItinClassIndx, unsigned OperandIdx) const {
+  std::optional<unsigned> getOperandCycle(unsigned ItinClassIndx,
+                                          unsigned OperandIdx) const {
     if (isEmpty())
-      return -1;
+      return std::nullopt;
 
     unsigned FirstIdx = Itineraries[ItinClassIndx].FirstOperandCycle;
     unsigned LastIdx = Itineraries[ItinClassIndx].LastOperandCycle;
     if ((FirstIdx + OperandIdx) >= LastIdx)
-      return -1;
+      return std::nullopt;
 
-    return (int)OperandCycles[FirstIdx + OperandIdx];
+    return OperandCycles[FirstIdx + OperandIdx];
   }
 
   /// Return true if there is a pipeline forwarding between instructions
@@ -202,24 +204,25 @@ class InstrItineraryData {
   /// Compute and return the use operand latency of a given itinerary
   /// class and operand index if the value is produced by an instruction of the
   /// specified itinerary class and def operand index.
-  int getOperandLatency(unsigned DefClass, unsigned DefIdx,
-                        unsigned UseClass, unsigned UseIdx) const {
+  std::optional<unsigned> getOperandLatency(unsigned DefClass, unsigned DefIdx,
+                                            unsigned UseClass,
+                                            unsigned UseIdx) const {
     if (isEmpty())
-      return -1;
+      return std::nullopt;
 
-    int DefCycle = getOperandCycle(DefClass, DefIdx);
-    if (DefCycle == -1)
-      return -1;
+    std::optional<unsigned> DefCycle = getOperandCycle(DefClass, DefIdx);
+    std::optional<unsigned> UseCycle = getOperandCycle(UseClass, UseIdx);
+    if (!DefCycle || !UseCycle)
+      return std::nullopt;
 
-    int UseCycle = getOperandCycle(UseClass, UseIdx);
-    if (UseCycle == -1)
-      return -1;
+    if (UseCycle > *DefCycle + 1)
+      return std::nullopt;
 
-    UseCycle = DefCycle - UseCycle + 1;
+    UseCycle = *DefCycle - *UseCycle + 1;
     if (UseCycle > 0 &&
         hasPipelineForwarding(DefClass, DefIdx, UseClass, UseIdx))
       // FIXME: This assumes one cycle benefit for every pipeline forwarding.
-      --UseCycle;
+      UseCycle = *UseCycle - 1;
     return UseCycle;
   }
 
diff --git a/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp b/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
index 0579c1664d5c9ab..4d6d350c46f5aff 100644
--- a/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
@@ -659,7 +659,8 @@ void ScheduleDAGSDNodes::computeOperandLatency(SDNode *Def, SDNode *Use,
   if (Use->isMachineOpcode())
     // Adjust the use operand index by num of defs.
     OpIdx += TII->get(Use->getMachineOpcode()).getNumDefs();
-  int Latency = TII->getOperandLatency(InstrItins, Def, DefIdx, Use, OpIdx);
+  std::optional<unsigned> Latency =
+      TII->getOperandLatency(InstrItins, Def, DefIdx, Use, OpIdx);
   if (Latency > 1 && Use->getOpcode() == ISD::CopyToReg &&
       !BB->succ_empty()) {
     unsigned Reg = cast<RegisterSDNode>(Use->getOperand(1))->getReg();
@@ -667,10 +668,10 @@ void ScheduleDAGSDNodes::computeOperandLatency(SDNode *Def, SDNode *Use,
       // This copy is a liveout value. It is likely coalesced, so reduce the
       // latency so not to penalize the def.
       // FIXME: need target specific adjustment here?
-      Latency = Latency - 1;
+      Latency = *Latency - 1;
   }
-  if (Latency >= 0)
-    dep.setLatency(Latency);
+  if (Latency)
+    dep.setLatency(*Latency);
 }
 
 void ScheduleDAGSDNodes::dumpNode(const SUnit &SU) const {
diff --git a/llvm/lib/CodeGen/TargetInstrInfo.cpp b/llvm/lib/CodeGen/TargetInstrInfo.cpp
index 4e4c43a19d11267..0b60f38529225be 100644
--- a/llvm/lib/CodeGen/TargetInstrInfo.cpp
+++ b/llvm/lib/CodeGen/TargetInstrInfo.cpp
@@ -1370,15 +1370,15 @@ bool TargetInstrInfo::getMemOperandWithOffset(
 //  SelectionDAG latency interface.
 //===----------------------------------------------------------------------===//
 
-int
+std::optional<unsigned>
 TargetInstrInfo::getOperandLatency(const InstrItineraryData *ItinData,
                                    SDNode *DefNode, unsigned DefIdx,
                                    SDNode *UseNode, unsigned UseIdx) const {
   if (!ItinData || ItinData->isEmpty())
-    return -1;
+    return std::nullopt;
 
   if (!DefNode->isMachineOpcode())
-    return -1;
+    return std::nullopt;
 
   unsigned DefClass = get(DefNode->getMachineOpcode()).getSchedClass();
   if (!UseNode->isMachineOpcode())
@@ -1387,8 +1387,8 @@ TargetInstrInfo::getOperandLatency(const InstrItineraryData *ItinData,
   return ItinData->getOperandLatency(DefClass, DefIdx, UseClass, UseIdx);
 }
 
-int TargetInstrInfo::getInstrLatency(const InstrItineraryData *ItinData,
-                                     SDNode *N) const {
+unsigned TargetInstrInfo::getInstrLatency(const InstrItineraryData *ItinData,
+                                          SDNode *N) const {
   if (!ItinData || ItinData->isEmpty())
     return 1;
 
@@ -1452,8 +1452,9 @@ bool TargetInstrInfo::hasLowDefLatency(const TargetSchedModel &SchedModel,
     return false;
 
   unsigned DefClass = DefMI.getDesc().getSchedClass();
-  int DefCycle = ItinData->getOperandCycle(DefClass, DefIdx);
-  return (DefCycle != -1 && DefCycle <= 1);
+  std::optional<unsigned> DefCycle =
+      ItinData->getOperandCycle(DefClass, DefIdx);
+  return DefCycle <= 1;
 }
 
 bool TargetInstrInfo::isFunctionSafeToSplit(const MachineFunction &MF) const {
@@ -1571,11 +1572,9 @@ unsigned TargetInstrInfo::getCallFrameSizeAt(MachineInstr &MI) const {
 
 /// Both DefMI and UseMI must be valid.  By default, call directly to the
 /// itinerary. This may be overriden by the target.
-int TargetInstrInfo::getOperandLatency(const InstrItineraryData *ItinData,
-                                       const MachineInstr &DefMI,
-                                       unsigned DefIdx,
-                                       const MachineInstr &UseMI,
-                                       unsigned UseIdx) const {
+std::optional<unsigned> TargetInstrInfo::getOperandLatency(
+    const InstrItineraryData *ItinData, const MachineInstr &DefMI,
+    unsigned DefIdx, const MachineInstr &UseMI, unsigned UseIdx) const {
   unsigned DefClass = DefMI.getDesc().getSchedClass();
   unsigned UseClass = UseMI.getDesc().getSchedClass();
   return ItinData->getOperandLatency(DefClass, DefIdx, UseClass, UseIdx);
diff --git a/llvm/lib/CodeGen/TargetSchedule.cpp b/llvm/lib/CodeGen/TargetSchedule.cpp
index 3cedb38de2ad8d9..549e3277f2671ba 100644
--- a/llvm/lib/CodeGen/TargetSchedule.cpp
+++ b/llvm/lib/CodeGen/TargetSchedule.cpp
@@ -173,11 +173,14 @@ unsigned TargetSchedModel::computeOperandLatency(
   const MachineInstr *DefMI, unsigned DefOperIdx,
   const MachineInstr *UseMI, unsigned UseOperIdx) const {
 
+  const unsigned InstrLatency = computeInstrLatency(DefMI);
+  const unsigned DefaultDefLatency = TII->defaultDefLatency(SchedModel, *DefMI);
+
   if (!hasInstrSchedModel() && !hasInstrItineraries())
-    return TII->defaultDefLatency(SchedModel, *DefMI);
+    return InstrLatency;
 
   if (hasInstrItineraries()) {
-    int OperLatency = 0;
+    std::optional<unsigned> OperLatency = 0;
     if (UseMI) {
       OperLatency = TII->getOperandLatency(&InstrItins, *DefMI, DefOperIdx,
                                            *UseMI, UseOperIdx);
@@ -186,21 +189,13 @@ unsigned TargetSchedModel::computeOperandLatency(
       unsigned DefClass = DefMI->getDesc().getSchedClass();
       OperLatency = InstrItins.getOperandCycle(DefClass, DefOperIdx);
     }
-    if (OperLatency >= 0)
-      return OperLatency;
-
-    // No operand latency was found.
-    unsigned InstrLatency = TII->getInstrLatency(&InstrItins, *DefMI);
-
-    // Expected latency is the max of the stage latency and itinerary props.
-    // Rather than directly querying InstrItins stage latency, we call a TII
-    // hook to allow subtargets to specialize latency. This hook is only
-    // applicable to the InstrItins model. InstrSchedModel should model all
-    // special cases without TII hooks.
-    InstrLatency =
-        std::max(InstrLatency, TII->defaultDefLatency(SchedModel, *DefMI));
-    return InstrLatency;
+
+    // Expected latency is the max of InstrLatency and DefaultDefLatency, if we
+    // didn't find an operand latency.
+    return OperLatency ? *OperLatency
+                       : std::max(InstrLatency, DefaultDefLatency);
   }
+
   // hasInstrSchedModel()
   const MCSchedClassDesc *SCDesc = resolveSchedClass(DefMI);
   unsigned DefIdx = findDefIdx(DefMI, DefOperIdx);
@@ -237,7 +232,7 @@ unsigned TargetSchedModel::computeOperandLatency(
   // FIXME: Automatically giving all implicit defs defaultDefLatency is
   // undesirable. We should only do it for defs that are known to the MC
   // desc like flags. Truly implicit defs should get 1 cycle latency.
-  return DefMI->isTransient() ? 0 : TII->defaultDefLatency(SchedModel, *DefMI);
+  return DefMI->isTransient() ? 0 : DefaultDefLatency;
 }
 
 unsigned
diff --git a/llvm/lib/MC/MCDisassembler/Disassembler.cpp b/llvm/lib/MC/MCDisassembler/Disassembler.cpp
index 067b951fbfccb38..5e5a163c290244d 100644
--- a/llvm/lib/MC/MCDisassembler/Disassembler.cpp
+++ b/llvm/lib/MC/MCDisassembler/Disassembler.cpp
@@ -180,12 +180,13 @@ static int getItineraryLatency(LLVMDisasmContext *DC, const MCInst &Inst) {
   const MCInstrDesc& Desc = DC->getInstrInfo()->get(Inst.getOpcode());
   unsigned SCClass = Desc.getSchedClass();
 
-  int Latency = 0;
-  for (unsigned OpIdx = 0, OpIdxEnd = Inst.getNumOperands(); OpIdx != OpIdxEnd;
-       ++OpIdx)
-    Latency = std::max(Latency, IID.getOperandCycle(SCClass, OpIdx));
+  unsigned Latency = 0;
 
-  return Latency;
+  for (unsigned Idx = 0, IdxEnd = Inst.getNumOperands(); Idx != IdxEnd; ++Idx)
+    if (std::optional<unsigned> OperCycle = IID.getOperandCycle(SCClass, Idx))
+      Latency = std::max(Latency, *OperCycle);
+
+  return (int)Latency;
 }
 
 /// Gets latency information for \p Inst, based on \p DC information.
diff --git a/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp b/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
index c09879fd9c2beb3..9c08d1e723ed35b 100644
--- a/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
+++ b/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
@@ -3872,17 +3872,16 @@ unsigned ARMBaseInstrInfo::getNumMicroOps(const InstrItineraryData *ItinData,
   llvm_unreachable("Didn't find the number of microops");
 }
 
-int
+std::optional<unsigned>
 ARMBaseInstrInfo::getVLDMDefCycle(const InstrItineraryData *ItinData,
-                                  const MCInstrDesc &DefMCID,
-                                  unsigned DefClass,
+                                  const MCInstrDesc &DefMCID, unsigned DefClass,
                                   unsigned DefIdx, unsigned DefAlign) const {
   int RegNo = (int)(DefIdx+1) - DefMCID.getNumOperands() + 1;
   if (RegNo <= 0)
     // Def is the address writeback.
     return ItinData->getOperandCycle(DefClass, DefIdx);
 
-  int DefCycle;
+  unsigned DefCycle;
   if (Subtarget.isCortexA8() || Subtarget.isCortexA7()) {
     // (regno / 2) + (regno % 2) + 1
     DefCycle = RegNo / 2 + 1;
@@ -3913,17 +3912,16 @@ ARMBaseInstrInfo::getVLDMDefCycle(const InstrItineraryData *ItinData,
   return DefCycle;
 }
 
-int
+std::optional<unsigned>
 ARMBaseInstrInfo::getLDMDefCycle(const InstrItineraryData *ItinData,
-                                 const MCInstrDesc &DefMCID,
-                                 unsigned DefClass,
+                                 const MCInstrDesc &DefMCID, unsigned DefClass,
                                  unsigned DefIdx, unsigned DefAlign) const {
   int RegNo = (int)(DefIdx+1) - DefMCID.getNumOperands() + 1;
   if (RegNo <= 0)
     // Def is the address writeback.
     return ItinData->getOperandCycle(DefClass, DefIdx);
 
-  int DefCycle;
+  unsigned DefCycle;
   if (Subtarget.isCortexA8() || Subtarget.isCortexA7()) {
     // 4 registers would be issued: 1, 2, 1.
     // 5 registers would be issued: 1, 2, 2.
@@ -3948,16 +3946,15 @@ ARMBaseInstrInfo::getLDMDefCycle(const InstrItineraryData *ItinData,
   return DefCycle;
 }
 
-int
+std::optional<unsigned>
 ARMBaseInstrInfo::getVSTMUseCycle(const InstrItineraryData *ItinData,
-                                  const MCInstrDesc &UseMCID,
-                                  unsigned UseClass,
+                                  const MCInstrDesc &UseMCID, unsigned UseClass,
                                   unsigned UseIdx, unsigned UseAlign) const {
   int RegNo = (int)(UseIdx+1) - UseMCID.getNumOperands() + 1;
   if (RegNo <= 0)
     return ItinData->getOperandCycle(UseClass, UseIdx);
 
-  int UseCycle;
+  unsigned UseCycle;
   if (Subtarget.isCortexA8() || Subtarget.isCortexA7()) {
     // (regno / 2) + (regno % 2) + 1
     UseCycle = RegNo / 2 + 1;
@@ -3988,16 +3985,15 @@ ARMBaseInstrInfo::getVSTMUseCycle(const InstrItineraryData *ItinData,
   return UseCycle;
 }
 
-int
+std::optional<unsigned>
 ARMBaseInstrInfo::getSTMUseCycle(const InstrItineraryData *ItinData,
-                                 const MCInstrDesc &UseMCID,
-                                 unsigned UseClass,
+                                 const MCInstrDesc &UseMCID, unsigned UseClass,
                                  unsigned UseIdx, unsigned UseAlign) const {
   int RegNo = (int)(UseIdx+1) - UseMCID.getNumOperands() + 1;
   if (RegNo <= 0)
     return ItinData->getOperandCycle(UseClass, UseIdx);
 
-  int UseCycle;
+  unsigned UseCycle;
   if (Subtarget.isCortexA8() || Subtarget.isCortexA7()) {
     UseCycle = RegNo / 2;
     if (UseCycle < 2)
@@ -4017,12 +4013,10 @@ ARMBaseInstrInfo::getSTMUseCycle(const InstrItineraryData *ItinData,
   return UseCycle;
 }
 
-int
-ARMBaseInstrInfo::getOperandLatency(const InstrItineraryData *ItinData,
-                                    const MCInstrDesc &DefMCID,
-                                    unsigned DefIdx, unsigned DefAlign,
-                                    const MCInstrDesc &UseMCID,
-                                    unsigned UseIdx, unsigned UseAlign) const {
+std::optional<unsigned> ARMBaseInstrInfo::getOperandLatency(
+    const InstrItineraryData *ItinData, const MCInstrDesc &DefMCID,
+    unsigned DefIdx, unsigned DefAlign, const MCInstrDesc &UseMCID,
+    unsigned UseIdx, unsigned UseAlign) const {
   unsigned DefClass = DefMCID.getSchedClass();
   unsigned UseClass = UseMCID.getSchedClass();
 
@@ -4032,7 +4026,7 @@ ARMBaseInstrInfo::getOperandLatency(const InstrItineraryData *ItinData,
   // This may be a def / use of a variable_ops instruction, the operand
   // latency might be determinable dynamically. Let the target try to
   // figure it out.
-  int DefCycle = -1;
+  std::optional<unsigned> DefCycle = std::nullopt;
   bool LdmBypass = false;
   switch (DefMCID.getOpcode()) {
   default:
@@ -4070,11 +4064,11 @@ ARMBaseInstrInfo::getOperandLatency(const InstrItineraryData *ItinData,
     break;
   }
 
-  if (DefCycle == -1)
+  if (!DefCycle)
     // We can't seem to determine the result latency of the def, assume it's 2.
     DefCycle = 2;
 
-  int UseCycle = -1;
+  std::optional<unsigned> UseCycle = std::nullopt;
   switch (UseMCID.getOpcode()) {
   default:
     UseCycle = ItinData->getOperandCycle(UseClass, UseIdx);
@@ -4108,21 +4102,24 @@ ARMBaseInstrInfo::getOperandLatency(const InstrItineraryData *ItinData,
     break;
   }
 
-  if (UseCycle == -1)
+  if (!UseCycle)
     // Assume it's read in the first stage.
     UseCycle = 1;
 
-  UseCycle = DefCycle - UseCycle + 1;
+  if (UseCycle > *DefCycle + 1)
+    return std::nullopt;
+
+  UseCycle = *DefCycle - *UseCycle + 1;
   if (UseCycle > 0) {
     if (LdmBypass) {
       // It's a variable_ops instruction so we can't use DefIdx here. Just use
       // first def operand.
       if (ItinData->hasPipelineForwarding(DefClass, DefMCID.getNumOperands()-1,
                                           UseClass, UseIdx))
-        --UseCycle;
+        UseCycle = *UseCycle - 1;
     } else if (ItinData->hasPipelineForwarding(DefClass, DefIdx,
                                                UseClass, UseIdx)) {
-      --UseCycle;
+      UseCycle = *UseCycle - 1;
     }
   }
 
@@ -4362,14 +4359,12 @@ static int adjustDefLatency(const ARMSubtarget &Subtarget,
   return Adjust;
 }
 
-int ARMBaseInstrInfo::getOperandLatency(const InstrItineraryData *ItinData,
-                                        const MachineInstr &DefMI,
-                                        unsigned DefIdx,
-                                        const MachineInstr &UseMI,
-                                        unsigned UseIdx) const {
+std::optional<unsigned> ARMBaseInstrInfo::getOperandLatency(
+    const InstrItineraryData *ItinData, const MachineInstr &DefMI,
+    unsigned DefIdx, const MachineInstr &UseMI, unsigned UseIdx) const {
   // No operand latency. The caller may fall back to getInstrLatency.
   if (!ItinData || ItinData->isEmpty())
-    return -1;
+    return std::nullopt;
 
   const MachineOperand &DefMO = DefMI.getOperand(DefIdx);
   Register Reg = DefMO.getReg();
@@ -4390,7 +4385,7 @@ int ARMBaseInstrInfo::getOperandLatency(const InstrItineraryData *ItinData,
     ResolvedUseMI =
         getBundledUseMI(&getRegisterInfo(), UseMI, Reg, UseIdx, UseAdj);
     if (!ResolvedUseMI)
-      return -1;
+      return std::nullopt;
   }
 
   return getOperandLatencyImpl(
@@ -4398,7 +4393,7 @@ int ARMBaseInstrInfo::getOperandLatency(const InstrItineraryData *ItinData,
       Reg, *ResolvedUseMI, UseIdx, ResolvedUseMI->getDesc(), UseAdj);
 }
 
-int ARMBaseInstrInfo::getOperandL...
[truncated]

llvmbot · 2023-11-30T15:36:10Z

@llvm/pr-subscribers-backend-arm

Author: Ramkumar Ramachandra (artagnon)

Changes

Factor out DefaultDefLatency and InstrLatency in computeOperandLatency.

Patch is 40.82 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/73769.diff

13 Files Affected:

(modified) llvm/include/llvm/CodeGen/TargetInstrInfo.h (+9-9)
(modified) llvm/include/llvm/MC/MCInstrItineraries.h (+18-15)
(modified) llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp (+5-4)
(modified) llvm/lib/CodeGen/TargetInstrInfo.cpp (+11-12)
(modified) llvm/lib/CodeGen/TargetSchedule.cpp (+12-17)
(modified) llvm/lib/MC/MCDisassembler/Disassembler.cpp (+6-5)
(modified) llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp (+58-59)
(modified) llvm/lib/Target/ARM/ARMBaseInstrInfo.h (+39-37)
(modified) llvm/lib/Target/Hexagon/HexagonInstrInfo.cpp (+6-8)
(modified) llvm/lib/Target/Hexagon/HexagonInstrInfo.h (+5-4)
(modified) llvm/lib/Target/Hexagon/HexagonSubtarget.cpp (+14-13)
(modified) llvm/lib/Target/PowerPC/PPCInstrInfo.cpp (+11-12)
(modified) llvm/lib/Target/PowerPC/PPCInstrInfo.h (+9-7)

diff --git a/llvm/include/llvm/CodeGen/TargetInstrInfo.h b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
index de065849eaa6ebc..fbff66f4ce544ef 100644
--- a/llvm/include/llvm/CodeGen/TargetInstrInfo.h
+++ b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
@@ -1705,9 +1705,9 @@ class TargetInstrInfo : public MCInstrInfo {
     return Opcode <= TargetOpcode::COPY;
   }
 
-  virtual int getOperandLatency(const InstrItineraryData *ItinData,
-                                SDNode *DefNode, unsigned DefIdx,
-                                SDNode *UseNode, unsigned UseIdx) const;
+  virtual std::optional<unsigned>
+  getOperandLatency(const InstrItineraryData *ItinData, SDNode *DefNode,
+                    unsigned DefIdx, SDNode *UseNode, unsigned UseIdx) const;
 
   /// Compute and return the use operand latency of a given pair of def and use.
   /// In most cases, the static scheduling itinerary was enough to determine the
@@ -1717,10 +1717,10 @@ class TargetInstrInfo : public MCInstrInfo {
   /// This is a raw interface to the itinerary that may be directly overridden
   /// by a target. Use computeOperandLatency to get the best estimate of
   /// latency.
-  virtual int getOperandLatency(const InstrItineraryData *ItinData,
-                                const MachineInstr &DefMI, unsigned DefIdx,
-                                const MachineInstr &UseMI,
-                                unsigned UseIdx) const;
+  virtual std::optional<unsigned>
+  getOperandLatency(const InstrItineraryData *ItinData,
+                    const MachineInstr &DefMI, unsigned DefIdx,
+                    const MachineInstr &UseMI, unsigned UseIdx) const;
 
   /// Compute the instruction latency of a given instruction.
   /// If the instruction has higher cost when predicated, it's returned via
@@ -1731,8 +1731,8 @@ class TargetInstrInfo : public MCInstrInfo {
 
   virtual unsigned getPredicationCost(const MachineInstr &MI) const;
 
-  virtual int getInstrLatency(const InstrItineraryData *ItinData,
-                              SDNode *Node) const;
+  virtual unsigned getInstrLatency(const InstrItineraryData *ItinData,
+                                   SDNode *Node) const;
 
   /// Return the default expected latency for a def based on its opcode.
   unsigned defaultDefLatency(const MCSchedModel &SchedModel,
diff --git a/llvm/include/llvm/MC/MCInstrItineraries.h b/llvm/include/llvm/MC/MCInstrItineraries.h
index 652922feddc3384..adf2ae7df2b6215 100644
--- a/llvm/include/llvm/MC/MCInstrItineraries.h
+++ b/llvm/include/llvm/MC/MCInstrItineraries.h
@@ -17,6 +17,7 @@
 
 #include "llvm/MC/MCSchedule.h"
 #include <algorithm>
+#include <optional>
 
 namespace llvm {
 
@@ -164,16 +165,17 @@ class InstrItineraryData {
 
   /// Return the cycle for the given class and operand.  Return -1 if no
   /// cycle is specified for the operand.
-  int getOperandCycle(unsigned ItinClassIndx, unsigned OperandIdx) const {
+  std::optional<unsigned> getOperandCycle(unsigned ItinClassIndx,
+                                          unsigned OperandIdx) const {
     if (isEmpty())
-      return -1;
+      return std::nullopt;
 
     unsigned FirstIdx = Itineraries[ItinClassIndx].FirstOperandCycle;
     unsigned LastIdx = Itineraries[ItinClassIndx].LastOperandCycle;
     if ((FirstIdx + OperandIdx) >= LastIdx)
-      return -1;
+      return std::nullopt;
 
-    return (int)OperandCycles[FirstIdx + OperandIdx];
+    return OperandCycles[FirstIdx + OperandIdx];
   }
 
   /// Return true if there is a pipeline forwarding between instructions
@@ -202,24 +204,25 @@ class InstrItineraryData {
   /// Compute and return the use operand latency of a given itinerary
   /// class and operand index if the value is produced by an instruction of the
   /// specified itinerary class and def operand index.
-  int getOperandLatency(unsigned DefClass, unsigned DefIdx,
-                        unsigned UseClass, unsigned UseIdx) const {
+  std::optional<unsigned> getOperandLatency(unsigned DefClass, unsigned DefIdx,
+                                            unsigned UseClass,
+                                            unsigned UseIdx) const {
     if (isEmpty())
-      return -1;
+      return std::nullopt;
 
-    int DefCycle = getOperandCycle(DefClass, DefIdx);
-    if (DefCycle == -1)
-      return -1;
+    std::optional<unsigned> DefCycle = getOperandCycle(DefClass, DefIdx);
+    std::optional<unsigned> UseCycle = getOperandCycle(UseClass, UseIdx);
+    if (!DefCycle || !UseCycle)
+      return std::nullopt;
 
-    int UseCycle = getOperandCycle(UseClass, UseIdx);
-    if (UseCycle == -1)
-      return -1;
+    if (UseCycle > *DefCycle + 1)
+      return std::nullopt;
 
-    UseCycle = DefCycle - UseCycle + 1;
+    UseCycle = *DefCycle - *UseCycle + 1;
     if (UseCycle > 0 &&
         hasPipelineForwarding(DefClass, DefIdx, UseClass, UseIdx))
       // FIXME: This assumes one cycle benefit for every pipeline forwarding.
-      --UseCycle;
+      UseCycle = *UseCycle - 1;
     return UseCycle;
   }
 
diff --git a/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp b/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
index 0579c1664d5c9ab..4d6d350c46f5aff 100644
--- a/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
@@ -659,7 +659,8 @@ void ScheduleDAGSDNodes::computeOperandLatency(SDNode *Def, SDNode *Use,
   if (Use->isMachineOpcode())
     // Adjust the use operand index by num of defs.
     OpIdx += TII->get(Use->getMachineOpcode()).getNumDefs();
-  int Latency = TII->getOperandLatency(InstrItins, Def, DefIdx, Use, OpIdx);
+  std::optional<unsigned> Latency =
+      TII->getOperandLatency(InstrItins, Def, DefIdx, Use, OpIdx);
   if (Latency > 1 && Use->getOpcode() == ISD::CopyToReg &&
       !BB->succ_empty()) {
     unsigned Reg = cast<RegisterSDNode>(Use->getOperand(1))->getReg();
@@ -667,10 +668,10 @@ void ScheduleDAGSDNodes::computeOperandLatency(SDNode *Def, SDNode *Use,
       // This copy is a liveout value. It is likely coalesced, so reduce the
       // latency so not to penalize the def.
       // FIXME: need target specific adjustment here?
-      Latency = Latency - 1;
+      Latency = *Latency - 1;
   }
-  if (Latency >= 0)
-    dep.setLatency(Latency);
+  if (Latency)
+    dep.setLatency(*Latency);
 }
 
 void ScheduleDAGSDNodes::dumpNode(const SUnit &SU) const {
diff --git a/llvm/lib/CodeGen/TargetInstrInfo.cpp b/llvm/lib/CodeGen/TargetInstrInfo.cpp
index 4e4c43a19d11267..0b60f38529225be 100644
--- a/llvm/lib/CodeGen/TargetInstrInfo.cpp
+++ b/llvm/lib/CodeGen/TargetInstrInfo.cpp
@@ -1370,15 +1370,15 @@ bool TargetInstrInfo::getMemOperandWithOffset(
 //  SelectionDAG latency interface.
 //===----------------------------------------------------------------------===//
 
-int
+std::optional<unsigned>
 TargetInstrInfo::getOperandLatency(const InstrItineraryData *ItinData,
                                    SDNode *DefNode, unsigned DefIdx,
                                    SDNode *UseNode, unsigned UseIdx) const {
   if (!ItinData || ItinData->isEmpty())
-    return -1;
+    return std::nullopt;
 
   if (!DefNode->isMachineOpcode())
-    return -1;
+    return std::nullopt;
 
   unsigned DefClass = get(DefNode->getMachineOpcode()).getSchedClass();
   if (!UseNode->isMachineOpcode())
@@ -1387,8 +1387,8 @@ TargetInstrInfo::getOperandLatency(const InstrItineraryData *ItinData,
   return ItinData->getOperandLatency(DefClass, DefIdx, UseClass, UseIdx);
 }
 
-int TargetInstrInfo::getInstrLatency(const InstrItineraryData *ItinData,
-                                     SDNode *N) const {
+unsigned TargetInstrInfo::getInstrLatency(const InstrItineraryData *ItinData,
+                                          SDNode *N) const {
   if (!ItinData || ItinData->isEmpty())
     return 1;
 
@@ -1452,8 +1452,9 @@ bool TargetInstrInfo::hasLowDefLatency(const TargetSchedModel &SchedModel,
     return false;
 
   unsigned DefClass = DefMI.getDesc().getSchedClass();
-  int DefCycle = ItinData->getOperandCycle(DefClass, DefIdx);
-  return (DefCycle != -1 && DefCycle <= 1);
+  std::optional<unsigned> DefCycle =
+      ItinData->getOperandCycle(DefClass, DefIdx);
+  return DefCycle <= 1;
 }
 
 bool TargetInstrInfo::isFunctionSafeToSplit(const MachineFunction &MF) const {
@@ -1571,11 +1572,9 @@ unsigned TargetInstrInfo::getCallFrameSizeAt(MachineInstr &MI) const {
 
 /// Both DefMI and UseMI must be valid.  By default, call directly to the
 /// itinerary. This may be overriden by the target.
-int TargetInstrInfo::getOperandLatency(const InstrItineraryData *ItinData,
-                                       const MachineInstr &DefMI,
-                                       unsigned DefIdx,
-                                       const MachineInstr &UseMI,
-                                       unsigned UseIdx) const {
+std::optional<unsigned> TargetInstrInfo::getOperandLatency(
+    const InstrItineraryData *ItinData, const MachineInstr &DefMI,
+    unsigned DefIdx, const MachineInstr &UseMI, unsigned UseIdx) const {
   unsigned DefClass = DefMI.getDesc().getSchedClass();
   unsigned UseClass = UseMI.getDesc().getSchedClass();
   return ItinData->getOperandLatency(DefClass, DefIdx, UseClass, UseIdx);
diff --git a/llvm/lib/CodeGen/TargetSchedule.cpp b/llvm/lib/CodeGen/TargetSchedule.cpp
index 3cedb38de2ad8d9..549e3277f2671ba 100644
--- a/llvm/lib/CodeGen/TargetSchedule.cpp
+++ b/llvm/lib/CodeGen/TargetSchedule.cpp
@@ -173,11 +173,14 @@ unsigned TargetSchedModel::computeOperandLatency(
   const MachineInstr *DefMI, unsigned DefOperIdx,
   const MachineInstr *UseMI, unsigned UseOperIdx) const {
 
+  const unsigned InstrLatency = computeInstrLatency(DefMI);
+  const unsigned DefaultDefLatency = TII->defaultDefLatency(SchedModel, *DefMI);
+
   if (!hasInstrSchedModel() && !hasInstrItineraries())
-    return TII->defaultDefLatency(SchedModel, *DefMI);
+    return InstrLatency;
 
   if (hasInstrItineraries()) {
-    int OperLatency = 0;
+    std::optional<unsigned> OperLatency = 0;
     if (UseMI) {
       OperLatency = TII->getOperandLatency(&InstrItins, *DefMI, DefOperIdx,
                                            *UseMI, UseOperIdx);
@@ -186,21 +189,13 @@ unsigned TargetSchedModel::computeOperandLatency(
       unsigned DefClass = DefMI->getDesc().getSchedClass();
       OperLatency = InstrItins.getOperandCycle(DefClass, DefOperIdx);
     }
-    if (OperLatency >= 0)
-      return OperLatency;
-
-    // No operand latency was found.
-    unsigned InstrLatency = TII->getInstrLatency(&InstrItins, *DefMI);
-
-    // Expected latency is the max of the stage latency and itinerary props.
-    // Rather than directly querying InstrItins stage latency, we call a TII
-    // hook to allow subtargets to specialize latency. This hook is only
-    // applicable to the InstrItins model. InstrSchedModel should model all
-    // special cases without TII hooks.
-    InstrLatency =
-        std::max(InstrLatency, TII->defaultDefLatency(SchedModel, *DefMI));
-    return InstrLatency;
+
+    // Expected latency is the max of InstrLatency and DefaultDefLatency, if we
+    // didn't find an operand latency.
+    return OperLatency ? *OperLatency
+                       : std::max(InstrLatency, DefaultDefLatency);
   }
+
   // hasInstrSchedModel()
   const MCSchedClassDesc *SCDesc = resolveSchedClass(DefMI);
   unsigned DefIdx = findDefIdx(DefMI, DefOperIdx);
@@ -237,7 +232,7 @@ unsigned TargetSchedModel::computeOperandLatency(
   // FIXME: Automatically giving all implicit defs defaultDefLatency is
   // undesirable. We should only do it for defs that are known to the MC
   // desc like flags. Truly implicit defs should get 1 cycle latency.
-  return DefMI->isTransient() ? 0 : TII->defaultDefLatency(SchedModel, *DefMI);
+  return DefMI->isTransient() ? 0 : DefaultDefLatency;
 }
 
 unsigned
diff --git a/llvm/lib/MC/MCDisassembler/Disassembler.cpp b/llvm/lib/MC/MCDisassembler/Disassembler.cpp
index 067b951fbfccb38..5e5a163c290244d 100644
--- a/llvm/lib/MC/MCDisassembler/Disassembler.cpp
+++ b/llvm/lib/MC/MCDisassembler/Disassembler.cpp
@@ -180,12 +180,13 @@ static int getItineraryLatency(LLVMDisasmContext *DC, const MCInst &Inst) {
   const MCInstrDesc& Desc = DC->getInstrInfo()->get(Inst.getOpcode());
   unsigned SCClass = Desc.getSchedClass();
 
-  int Latency = 0;
-  for (unsigned OpIdx = 0, OpIdxEnd = Inst.getNumOperands(); OpIdx != OpIdxEnd;
-       ++OpIdx)
-    Latency = std::max(Latency, IID.getOperandCycle(SCClass, OpIdx));
+  unsigned Latency = 0;
 
-  return Latency;
+  for (unsigned Idx = 0, IdxEnd = Inst.getNumOperands(); Idx != IdxEnd; ++Idx)
+    if (std::optional<unsigned> OperCycle = IID.getOperandCycle(SCClass, Idx))
+      Latency = std::max(Latency, *OperCycle);
+
+  return (int)Latency;
 }
 
 /// Gets latency information for \p Inst, based on \p DC information.
diff --git a/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp b/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
index c09879fd9c2beb3..9c08d1e723ed35b 100644
--- a/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
+++ b/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
@@ -3872,17 +3872,16 @@ unsigned ARMBaseInstrInfo::getNumMicroOps(const InstrItineraryData *ItinData,
   llvm_unreachable("Didn't find the number of microops");
 }
 
-int
+std::optional<unsigned>
 ARMBaseInstrInfo::getVLDMDefCycle(const InstrItineraryData *ItinData,
-                                  const MCInstrDesc &DefMCID,
-                                  unsigned DefClass,
+                                  const MCInstrDesc &DefMCID, unsigned DefClass,
                                   unsigned DefIdx, unsigned DefAlign) const {
   int RegNo = (int)(DefIdx+1) - DefMCID.getNumOperands() + 1;
   if (RegNo <= 0)
     // Def is the address writeback.
     return ItinData->getOperandCycle(DefClass, DefIdx);
 
-  int DefCycle;
+  unsigned DefCycle;
   if (Subtarget.isCortexA8() || Subtarget.isCortexA7()) {
     // (regno / 2) + (regno % 2) + 1
     DefCycle = RegNo / 2 + 1;
@@ -3913,17 +3912,16 @@ ARMBaseInstrInfo::getVLDMDefCycle(const InstrItineraryData *ItinData,
   return DefCycle;
 }
 
-int
+std::optional<unsigned>
 ARMBaseInstrInfo::getLDMDefCycle(const InstrItineraryData *ItinData,
-                                 const MCInstrDesc &DefMCID,
-                                 unsigned DefClass,
+                                 const MCInstrDesc &DefMCID, unsigned DefClass,
                                  unsigned DefIdx, unsigned DefAlign) const {
   int RegNo = (int)(DefIdx+1) - DefMCID.getNumOperands() + 1;
   if (RegNo <= 0)
     // Def is the address writeback.
     return ItinData->getOperandCycle(DefClass, DefIdx);
 
-  int DefCycle;
+  unsigned DefCycle;
   if (Subtarget.isCortexA8() || Subtarget.isCortexA7()) {
     // 4 registers would be issued: 1, 2, 1.
     // 5 registers would be issued: 1, 2, 2.
@@ -3948,16 +3946,15 @@ ARMBaseInstrInfo::getLDMDefCycle(const InstrItineraryData *ItinData,
   return DefCycle;
 }
 
-int
+std::optional<unsigned>
 ARMBaseInstrInfo::getVSTMUseCycle(const InstrItineraryData *ItinData,
-                                  const MCInstrDesc &UseMCID,
-                                  unsigned UseClass,
+                                  const MCInstrDesc &UseMCID, unsigned UseClass,
                                   unsigned UseIdx, unsigned UseAlign) const {
   int RegNo = (int)(UseIdx+1) - UseMCID.getNumOperands() + 1;
   if (RegNo <= 0)
     return ItinData->getOperandCycle(UseClass, UseIdx);
 
-  int UseCycle;
+  unsigned UseCycle;
   if (Subtarget.isCortexA8() || Subtarget.isCortexA7()) {
     // (regno / 2) + (regno % 2) + 1
     UseCycle = RegNo / 2 + 1;
@@ -3988,16 +3985,15 @@ ARMBaseInstrInfo::getVSTMUseCycle(const InstrItineraryData *ItinData,
   return UseCycle;
 }
 
-int
+std::optional<unsigned>
 ARMBaseInstrInfo::getSTMUseCycle(const InstrItineraryData *ItinData,
-                                 const MCInstrDesc &UseMCID,
-                                 unsigned UseClass,
+                                 const MCInstrDesc &UseMCID, unsigned UseClass,
                                  unsigned UseIdx, unsigned UseAlign) const {
   int RegNo = (int)(UseIdx+1) - UseMCID.getNumOperands() + 1;
   if (RegNo <= 0)
     return ItinData->getOperandCycle(UseClass, UseIdx);
 
-  int UseCycle;
+  unsigned UseCycle;
   if (Subtarget.isCortexA8() || Subtarget.isCortexA7()) {
     UseCycle = RegNo / 2;
     if (UseCycle < 2)
@@ -4017,12 +4013,10 @@ ARMBaseInstrInfo::getSTMUseCycle(const InstrItineraryData *ItinData,
   return UseCycle;
 }
 
-int
-ARMBaseInstrInfo::getOperandLatency(const InstrItineraryData *ItinData,
-                                    const MCInstrDesc &DefMCID,
-                                    unsigned DefIdx, unsigned DefAlign,
-                                    const MCInstrDesc &UseMCID,
-                                    unsigned UseIdx, unsigned UseAlign) const {
+std::optional<unsigned> ARMBaseInstrInfo::getOperandLatency(
+    const InstrItineraryData *ItinData, const MCInstrDesc &DefMCID,
+    unsigned DefIdx, unsigned DefAlign, const MCInstrDesc &UseMCID,
+    unsigned UseIdx, unsigned UseAlign) const {
   unsigned DefClass = DefMCID.getSchedClass();
   unsigned UseClass = UseMCID.getSchedClass();
 
@@ -4032,7 +4026,7 @@ ARMBaseInstrInfo::getOperandLatency(const InstrItineraryData *ItinData,
   // This may be a def / use of a variable_ops instruction, the operand
   // latency might be determinable dynamically. Let the target try to
   // figure it out.
-  int DefCycle = -1;
+  std::optional<unsigned> DefCycle = std::nullopt;
   bool LdmBypass = false;
   switch (DefMCID.getOpcode()) {
   default:
@@ -4070,11 +4064,11 @@ ARMBaseInstrInfo::getOperandLatency(const InstrItineraryData *ItinData,
     break;
   }
 
-  if (DefCycle == -1)
+  if (!DefCycle)
     // We can't seem to determine the result latency of the def, assume it's 2.
     DefCycle = 2;
 
-  int UseCycle = -1;
+  std::optional<unsigned> UseCycle = std::nullopt;
   switch (UseMCID.getOpcode()) {
   default:
     UseCycle = ItinData->getOperandCycle(UseClass, UseIdx);
@@ -4108,21 +4102,24 @@ ARMBaseInstrInfo::getOperandLatency(const InstrItineraryData *ItinData,
     break;
   }
 
-  if (UseCycle == -1)
+  if (!UseCycle)
     // Assume it's read in the first stage.
     UseCycle = 1;
 
-  UseCycle = DefCycle - UseCycle + 1;
+  if (UseCycle > *DefCycle + 1)
+    return std::nullopt;
+
+  UseCycle = *DefCycle - *UseCycle + 1;
   if (UseCycle > 0) {
     if (LdmBypass) {
       // It's a variable_ops instruction so we can't use DefIdx here. Just use
       // first def operand.
       if (ItinData->hasPipelineForwarding(DefClass, DefMCID.getNumOperands()-1,
                                           UseClass, UseIdx))
-        --UseCycle;
+        UseCycle = *UseCycle - 1;
     } else if (ItinData->hasPipelineForwarding(DefClass, DefIdx,
                                                UseClass, UseIdx)) {
-      --UseCycle;
+      UseCycle = *UseCycle - 1;
     }
   }
 
@@ -4362,14 +4359,12 @@ static int adjustDefLatency(const ARMSubtarget &Subtarget,
   return Adjust;
 }
 
-int ARMBaseInstrInfo::getOperandLatency(const InstrItineraryData *ItinData,
-                                        const MachineInstr &DefMI,
-                                        unsigned DefIdx,
-                                        const MachineInstr &UseMI,
-                                        unsigned UseIdx) const {
+std::optional<unsigned> ARMBaseInstrInfo::getOperandLatency(
+    const InstrItineraryData *ItinData, const MachineInstr &DefMI,
+    unsigned DefIdx, const MachineInstr &UseMI, unsigned UseIdx) const {
   // No operand latency. The caller may fall back to getInstrLatency.
   if (!ItinData || ItinData->isEmpty())
-    return -1;
+    return std::nullopt;
 
   const MachineOperand &DefMO = DefMI.getOperand(DefIdx);
   Register Reg = DefMO.getReg();
@@ -4390,7 +4385,7 @@ int ARMBaseInstrInfo::getOperandLatency(const InstrItineraryData *ItinData,
     ResolvedUseMI =
         getBundledUseMI(&getRegisterInfo(), UseMI, Reg, UseIdx, UseAdj);
     if (!ResolvedUseMI)
-      return -1;
+      return std::nullopt;
   }
 
   return getOperandLatencyImpl(
@@ -4398,7 +4393,7 @@ int ARMBaseInstrInfo::getOperandLatency(const InstrItineraryData *ItinData,
       Reg, *ResolvedUseMI, UseIdx, ResolvedUseMI->getDesc(), UseAdj);
 }
 
-int ARMBaseInstrInfo::getOperandL...
[truncated]

llvm/include/llvm/MC/MCInstrItineraries.h

fpetrogalli

One more round. Thanks!

llvm/include/llvm/CodeGen/TargetInstrInfo.h

llvm/include/llvm/MC/MCInstrItineraries.h

llvm/lib/CodeGen/TargetSchedule.cpp

llvm/include/llvm/MC/MCInstrItineraries.h

llvm/lib/MC/MCDisassembler/Disassembler.cpp

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp

llvm/lib/Target/Hexagon/HexagonSubtarget.cpp

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp

fpetrogalli

Thanks - just minor stuff on two comments.

llvm/include/llvm/MC/MCInstrItineraries.h

getOperandLatency has the following behavior: it returns -1 as a special value, negative numbers other than -1 on some target-specific overrides, or a valid non-negative latency. This behavior can be surprising, as some callers do arithmetic on these negative values. Change the interface of getOperandLatency to return a std::optional<unsigned> to prevent surprises in callers.

RKSimon · 2023-12-01T13:02:01Z

@artagnon I'm seeing a lot of signed/unsigned warnings on MSVC builds (and we build with WX/Werror by default) - please can you take a look?

https://lab.llvm.org/buildbot/#/builders/216/builds/31206

artagnon · 2023-12-01T13:20:46Z

I'm somewhat confused by the build log. The only places the error could possibly be coming from are:

  if (UseCycle > *DefCycle + 1)
    return std::nullopt;

Here, *DefCycle is unsigned, and unsigned + 1 = unsigned, so this shouldn't qualify.

The other place is:

    UseCycle = *DefCycle - *UseCycle + 1;
    if (UseCycle > 0 &&
        hasPipelineForwarding(DefClass, DefIdx, UseClass, UseIdx))

UseCycle is unsigned, and although MSVC could have inferred that its assignment would produce a negative value, it would technically be an unsigned-wrap. Besides, the error is coming from >, and I don't see how UseCycle > 0 could possibly qualify.

Could you try fixing up the first instance with an unsigned-cast, try it on MSVC, and tell me if the error still persists? If it doesn't, I'll push a fix.

RKSimon · 2023-12-01T13:31:04Z

[732/1490/3106/5327] Building CXX object lib\Targ...LVMMipsCodeGen.dir\MipsModuleISelDAGToDAG.cpp.obj
C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Tools\MSVC\14.38.33130\include\optional(883): warning C4018: '>': signed/unsigned mismatch
C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Tools\MSVC\14.38.33130\include\optional(883): note: the template instantiation context (the oldest one first) is
E:\llvm\llvm-project\llvm\include\llvm/MC/MCInstrItineraries.h(223): note: see reference to function template instantiation 'bool std::operator ><unsigned int,int,0>(const std::optional<unsigned int> &,const _Ty2 &) noexcept(<expr>)' being compiled
        with
        [
            _Ty2=int
        ]

artagnon · 2023-12-01T13:39:08Z

Ah yes, it should be UseCycles > 0u.

artagnon · 2023-12-01T13:53:40Z

Fixed in #74078.

llvm/lib/CodeGen/TargetSchedule.cpp

llvm/lib/CodeGen/TargetInstrInfo.cpp

artagnon · 2023-12-04T16:43:31Z

Thanks @bjope for the post-commit review. Issues have been fixed in #74338.

artagnon requested review from jayfoad, serge-sans-paille, davemgreen, nico, dsandersllvm and fpetrogalli November 29, 2023 09:14

fpetrogalli reviewed Nov 29, 2023

View reviewed changes

llvm/lib/CodeGen/TargetSchedule.cpp Outdated Show resolved Hide resolved

llvm/lib/CodeGen/TargetSchedule.cpp Outdated Show resolved Hide resolved

llvm/lib/CodeGen/TargetSchedule.cpp Outdated Show resolved Hide resolved

llvm/lib/CodeGen/TargetSchedule.cpp Outdated Show resolved Hide resolved

artagnon force-pushed the targetsched-cleanup branch from a442916 to 9009d86 Compare November 30, 2023 15:35

llvmbot added backend:ARM mc Machine (object) code llvm:SelectionDAG SelectionDAGISel as well labels Nov 30, 2023

artagnon changed the title ~~TargetSchedule: factor out code in computeOperandLatency (NFC)~~ TargetInstrInfo: make getOperandLatency return optional (NFC) Nov 30, 2023

artagnon requested a review from fpetrogalli November 30, 2023 15:37

RKSimon reviewed Nov 30, 2023

View reviewed changes

llvm/include/llvm/MC/MCInstrItineraries.h Show resolved Hide resolved

fpetrogalli requested changes Dec 1, 2023

View reviewed changes

artagnon requested a review from fpetrogalli December 1, 2023 09:35

fpetrogalli approved these changes Dec 1, 2023

View reviewed changes

llvm/include/llvm/MC/MCInstrItineraries.h Outdated Show resolved Hide resolved

llvm/include/llvm/MC/MCInstrItineraries.h Outdated Show resolved Hide resolved

artagnon force-pushed the targetsched-cleanup branch from 8f0ce20 to 7269c37 Compare December 1, 2023 10:19

artagnon merged commit 9468de4 into llvm:main Dec 1, 2023
3 checks passed

artagnon deleted the targetsched-cleanup branch December 1, 2023 11:29

bjope reviewed Dec 3, 2023

View reviewed changes

llvm/lib/CodeGen/TargetSchedule.cpp Show resolved Hide resolved

bjope reviewed Dec 3, 2023

View reviewed changes

llvm/lib/CodeGen/TargetSchedule.cpp Show resolved Hide resolved

bjope reviewed Dec 4, 2023

View reviewed changes

llvm/lib/CodeGen/TargetInstrInfo.cpp Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TargetInstrInfo: make getOperandLatency return optional (NFC) #73769

TargetInstrInfo: make getOperandLatency return optional (NFC) #73769

artagnon commented Nov 29, 2023 •

edited

fpetrogalli left a comment

github-actions bot commented Nov 30, 2023 •

edited

llvmbot commented Nov 30, 2023 •

edited

llvmbot commented Nov 30, 2023

fpetrogalli left a comment

fpetrogalli left a comment

RKSimon commented Dec 1, 2023 •

edited

artagnon commented Dec 1, 2023 •

edited

RKSimon commented Dec 1, 2023

artagnon commented Dec 1, 2023

artagnon commented Dec 1, 2023

artagnon commented Dec 4, 2023

TargetInstrInfo: make getOperandLatency return optional (NFC) #73769

TargetInstrInfo: make getOperandLatency return optional (NFC) #73769

Conversation

artagnon commented Nov 29, 2023 • edited

fpetrogalli left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 30, 2023 • edited

llvmbot commented Nov 30, 2023 • edited

llvmbot commented Nov 30, 2023

fpetrogalli left a comment

Choose a reason for hiding this comment

fpetrogalli left a comment

Choose a reason for hiding this comment

RKSimon commented Dec 1, 2023 • edited

artagnon commented Dec 1, 2023 • edited

RKSimon commented Dec 1, 2023

artagnon commented Dec 1, 2023

artagnon commented Dec 1, 2023

artagnon commented Dec 4, 2023

artagnon commented Nov 29, 2023 •

edited

github-actions bot commented Nov 30, 2023 •

edited

llvmbot commented Nov 30, 2023 •

edited

RKSimon commented Dec 1, 2023 •

edited

artagnon commented Dec 1, 2023 •

edited