TargetInstrInfo: make getOperandLatency return optional (NFC)

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.

Author: artagnon

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,

llvm/include/llvm/MC/MCInstrItineraries.h

@@ -17,6 +17,7 @@
 
 #include "llvm/MC/MCSchedule.h"
 #include <algorithm>
+#include <optional>
 
 namespace llvm {
 
@@ -162,18 +163,19 @@ class InstrItineraryData {
     return Latency;
   }
 
-  /// 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 {
+  /// Return the cycle for the given class and operand. Return std::nullopt if
+  /// the information is not available for the operand.
+  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
@@ -201,25 +203,27 @@ 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 {
+  /// specified itinerary class and def operand index. Return std::nullopt if
+  /// the information is not available for the operand.
+  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;
   }
 

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp

@@ -659,18 +659,19 @@ 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();
     if (Register::isVirtualRegister(Reg))
       // 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 {

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);

llvm/lib/CodeGen/TargetSchedule.cpp

@@ -168,16 +168,20 @@ static unsigned findUseIdx(const MachineInstr *MI, unsigned UseOperIdx) {
   return UseIdx;
 }
 
-// Top-level API for clients that know the operand indices.
+// Top-level API for clients that know the operand indices. This doesn't need to
+// return std::optional<unsigned>, as it always returns a valid latency.
 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;
     if (UseMI) {
       OperLatency = TII->getOperandLatency(&InstrItins, *DefMI, DefOperIdx,
                                            *UseMI, UseOperIdx);
@@ -186,21 +190,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 +233,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

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.