Skip to content

Conversation

artagnon
Copy link
Contributor

@artagnon artagnon commented Dec 4, 2023

Follow up on a post-commit review of 9468de4 (TargetInstrInfo: make getOperandLatency return optional (NFC)) by Bjorn Pettersson to fix a couple of things that are not NFC:

  • std::optional<T>::operator<= returns true if the first operand is a std::nullopt and second operand is T. Fix a couple of places where we assumed it would return false.
  • In TargetSchedule, computeInstrCost could take another codepath, returning InstrLatency instead of DefaultDefLatency. Fix one instance not accounting for this behavior.

Follow up on a post-commit review of 9468de4 (TargetInstrInfo: make
getOperandLatency return optional (NFC)) by Bjorn Pettersson to fix a
couple of things that are not NFC:

- std::optional::operator<= returns true if the first operand is a
  std::nullopt. Fix a couple of places where we assumed it would return
  false.
- In TargetSchedule, computeInstrCost could take another codepath,
  returning InstrLatency instead of DefaultDefLatency. Fix one instance
  not accounting for this behavior.
@artagnon artagnon requested a review from bjope December 4, 2023 16:42
@llvmbot
Copy link
Member

llvmbot commented Dec 4, 2023

@llvm/pr-subscribers-backend-arm

Author: Ramkumar Ramachandra (artagnon)

Changes

Follow up on a post-commit review of 9468de4 (TargetInstrInfo: make getOperandLatency return optional (NFC)) by Bjorn Pettersson to fix a couple of things that are not NFC:

  • std::optional::operator<= returns true if the first operand is a std::nullopt. Fix a couple of places where we assumed it would return false.
  • In TargetSchedule, computeInstrCost could take another codepath, returning InstrLatency instead of DefaultDefLatency. Fix one instance not accounting for this behavior.

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

3 Files Affected:

  • (modified) llvm/lib/CodeGen/TargetInstrInfo.cpp (+1-1)
  • (modified) llvm/lib/CodeGen/TargetSchedule.cpp (+1-1)
  • (modified) llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp (+1-1)
diff --git a/llvm/lib/CodeGen/TargetInstrInfo.cpp b/llvm/lib/CodeGen/TargetInstrInfo.cpp
index 4bd5c910b298d..4783742a14ad7 100644
--- a/llvm/lib/CodeGen/TargetInstrInfo.cpp
+++ b/llvm/lib/CodeGen/TargetInstrInfo.cpp
@@ -1462,7 +1462,7 @@ bool TargetInstrInfo::hasLowDefLatency(const TargetSchedModel &SchedModel,
   unsigned DefClass = DefMI.getDesc().getSchedClass();
   std::optional<unsigned> DefCycle =
       ItinData->getOperandCycle(DefClass, DefIdx);
-  return DefCycle <= 1U;
+  return DefCycle && DefCycle <= 1U;
 }
 
 bool TargetInstrInfo::isFunctionSafeToSplit(const MachineFunction &MF) const {
diff --git a/llvm/lib/CodeGen/TargetSchedule.cpp b/llvm/lib/CodeGen/TargetSchedule.cpp
index a25d4ff78f4d9..ce59b096992d8 100644
--- a/llvm/lib/CodeGen/TargetSchedule.cpp
+++ b/llvm/lib/CodeGen/TargetSchedule.cpp
@@ -178,7 +178,7 @@ unsigned TargetSchedModel::computeOperandLatency(
   const unsigned DefaultDefLatency = TII->defaultDefLatency(SchedModel, *DefMI);
 
   if (!hasInstrSchedModel() && !hasInstrItineraries())
-    return InstrLatency;
+    return DefaultDefLatency;
 
   if (hasInstrItineraries()) {
     std::optional<unsigned> OperLatency;
diff --git a/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp b/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
index 476a9bb15edbf..b85107ec47191 100644
--- a/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
+++ b/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
@@ -4836,7 +4836,7 @@ bool ARMBaseInstrInfo::hasLowDefLatency(const TargetSchedModel &SchedModel,
     unsigned DefClass = DefMI.getDesc().getSchedClass();
     std::optional<unsigned> DefCycle =
         ItinData->getOperandCycle(DefClass, DefIdx);
-    return DefCycle <= 2U;
+    return DefCycle && DefCycle <= 2U;
   }
   return false;
 }

@bjope
Copy link
Collaborator

bjope commented Dec 4, 2023

LGTM

Copy link
Collaborator

@bjope bjope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG

@artagnon artagnon merged commit e8dbe94 into llvm:main Dec 5, 2023
@artagnon artagnon deleted the non-nfc-fix branch December 5, 2023 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants