Rethink fix: Don't convert 0 to 1.

Author: jdenny-ornl

llvm/docs/LangRef.rst

@@ -7989,9 +7989,9 @@ this metadata is added (i.e., has been distributed).  See
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 This metadata records an estimated trip count for the loop.  The first operand
-is the string ``llvm.loop.estimated_trip_count``.  The second operand is a
-positive integer constant of type ``i32`` or smaller specifying the estimate.
-For example:
+is the string ``llvm.loop.estimated_trip_count``.  The second operand is an
+integer constant of type ``i32`` or smaller specifying the estimate.  For
+example:
 
 .. code-block:: llvm
 
@@ -8033,12 +8033,20 @@ pass should record the new estimates by calling
 loop, ``llvm::getLoopEstimatedTripCount`` returns its value instead of
 estimating the trip count from the loop's ``branch_weights`` metadata.
 
-Especially after a transformation like loop peeling, the probability of reaching
-a loop's header might be very low.  Regardless, in the case that it is reached,
-at least one iteration will execute, so an estimated trip count of zero is
-invalid.  Some passes thus rely on non-zero estimated trip counts.
-Nevertheless, some passes naively compute it as zero.  To avoid misbehavior,
-``llvm::setLoopEstimatedTripCount`` converts zero to one.
+Zero
+""""
+
+Some passes set ``llvm.loop.estimated_trip_count`` to 0.  For example, after
+peeling 10 or more iterations from a loop with an estimated trip count of 10,
+``llvm.loop.estimated_trip_count`` becomes 0 on the remaining loop.  It
+indicates that, each time execution reaches the peeled iterations, execution is
+estimated to exit them without reaching the remaining loop's header.
+
+Even if the probability of reaching a loop's header is low, if it is reached, it
+is the start of an iteration.  Consequently, some passes historically assume
+that ``llvm::getLoopEstimatedTripCount`` always returns a positive count or
+``std::nullopt``.  Thus, it returns ``std::nullopt`` when
+``llvm.loop.estimated_trip_count`` is 0.
 
 '``llvm.licm.disable``' Metadata
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

llvm/include/llvm/Transforms/Utils/LoopUtils.h

@@ -322,21 +322,14 @@ LLVM_ABI TransformationMode hasLICMVersioningTransformation(const Loop *L);
 LLVM_ABI void addStringMetadataToLoop(Loop *TheLoop, const char *MDString,
                                       unsigned V = 0);
 
-/// Set \p StringMD in the loop metadata of \p TheLoop while keeping other
-/// values intact:
-/// - If \p V is \c std::nullopt, remove \p StringMD, or do nothing if
-///   \p StringMD is not present, perhaps because there is no loop metadata.
-/// - Else, set \p StringMD to \p V.  Either add \p StringMD if absent, update
-///   it if the current value is different, or do nothing if the current value
-///   is the same.
-LLVM_ABI void setLoopStringMetadata(Loop *TheLoop, const char *StringMD,
-                                    std::optional<unsigned> V);
-
 /// Return either:
 /// - \c std::nullopt, if the implementation is unable to handle the loop form
 ///   of \p L (e.g., \p L must have a latch block that controls the loop exit).
 /// - The value of \c llvm.loop.estimated_trip_count from the loop metadata of
-///   \p L, if that metadata is present.
+///   \p L, if that metadata is present.  In the special case that the value is
+///   zero, return \c std::nullopt instead as that is historically what callers
+///   expect when a loop is estimated to execute no iterations (i.e., its header
+///   is not reached).
 /// - Else, a new estimate of the trip count from the latch branch weights of
 ///   \p L.
 ///
@@ -363,23 +356,15 @@ getLoopEstimatedTripCount(Loop *L,
 /// to handle the loop form of \p L (e.g., \p L must have a latch block that
 /// controls the loop exit).  Otherwise, return true.
 ///
-/// Some passes rely on estimated trip counts to be non-zero because, once a
-/// loop header is reached, at least one iteration will execute.  However, some
-/// passes naively compute it as zero.  To avoid misbehavior, if
-/// \p EstimatedTripCount is zero, interpret it as one.
-///
 /// In addition, if \p EstimatedLoopInvocationWeight:
 /// - Set the branch weight metadata of \p L to reflect that \p L has an
 ///   estimated \p EstimatedTripCount iterations and has
 ///   \c *EstimatedLoopInvocationWeight exit weight through the loop's latch.
-/// - If \p EstimatedTripCount is zero, zero the branch weights, and drop
-///   \c llvm.loop.estimated_trip_count entirely.  \c getLoopEstimatedTripCount
-///   will then return \c std::nullopt.
+/// - If \p EstimatedTripCount is zero, zero the branch weights.
 ///
 /// TODO: Eventually, once all passes have migrated away from setting branch
 /// weights to indicate estimated trip counts, this function will drop the
-/// \p EstimatedLoopInvocationWeight parameter, so its historical handling of
-/// \p EstimatedTripCount == 0 should no longer be needed.
+/// \p EstimatedLoopInvocationWeight parameter.
 LLVM_ABI bool setLoopEstimatedTripCount(
     Loop *L, unsigned EstimatedTripCount,
     std::optional<unsigned> EstimatedLoopInvocationWeight = std::nullopt);

llvm/lib/IR/Verifier.cpp

@@ -1094,10 +1094,9 @@ void Verifier::visitMDNode(const MDNode &MD, AreDebugLocsAllowed AllowLocs) {
     Check(MD.getNumOperands() == 2, "Expected two operands", &MD);
     auto *Count = dyn_cast_or_null<ConstantAsMetadata>(MD.getOperand(1));
     Check(Count && Count->getType()->isIntegerTy() &&
-              cast<IntegerType>(Count->getType())->getBitWidth() <= 32 &&
-              !cast<ConstantInt>(Count->getValue())->isZero(),
-          "Expected second operand to be a positive integer constant of type "
-          "i32 or smaller",
+              cast<IntegerType>(Count->getType())->getBitWidth() <= 32,
+          "Expected second operand to be an integer constant of type i32 or "
+          "smaller",
           &MD);
   }
 

llvm/lib/Transforms/Utils/LoopUtils.cpp

@@ -214,43 +214,31 @@ static MDNode *createStringMetadata(Loop *TheLoop, StringRef Name, unsigned V) {
 /// different.
 void llvm::addStringMetadataToLoop(Loop *TheLoop, const char *StringMD,
                                    unsigned V) {
-  setLoopStringMetadata(TheLoop, StringMD, V);
-}
-
-void llvm::setLoopStringMetadata(Loop *TheLoop, const char *StringMD,
-                                 std::optional<unsigned> V) {
   SmallVector<Metadata *, 4> MDs(1);
   // If the loop already has metadata, retain it.
   MDNode *LoopID = TheLoop->getLoopID();
-  bool Found = false;
   if (LoopID) {
     for (unsigned i = 1, ie = LoopID->getNumOperands(); i < ie; ++i) {
       MDNode *Node = cast<MDNode>(LoopID->getOperand(i));
       // If it is of form key = value, try to parse it.
       if (Node->getNumOperands() == 2) {
         MDString *S = dyn_cast<MDString>(Node->getOperand(0));
         if (S && S->getString() == StringMD) {
-          Found = true;
-          if (V) {
-            ConstantInt *IntMD =
-                mdconst::extract_or_null<ConstantInt>(Node->getOperand(1));
-            if (IntMD && IntMD->getSExtValue() == *V)
-              // It is already in place. Do nothing.
-              return;
-          }
-          // We need to update/remove the value, so just skip it here and it
-          // will be added/removed after copying other existed nodes.
+          ConstantInt *IntMD =
+              mdconst::extract_or_null<ConstantInt>(Node->getOperand(1));
+          if (IntMD && IntMD->getSExtValue() == V)
+            // It is already in place. Do nothing.
+            return;
+          // We need to update the value, so just skip it here and it will
+          // be added after copying other existed nodes.
           continue;
         }
       }
       MDs.push_back(Node);
     }
   }
-  if (!V && !Found)
-    return;
   // Add new metadata.
-  if (V)
-    MDs.push_back(createStringMetadata(TheLoop, StringMD, *V));
+  MDs.push_back(createStringMetadata(TheLoop, StringMD, V));
   // Replace current metadata node with new one.
   LLVMContext &Context = TheLoop->getHeader()->getContext();
   MDNode *NewLoopID = MDNode::get(Context, MDs);
@@ -924,13 +912,14 @@ llvm::getLoopEstimatedTripCount(Loop *L,
   }
 
   // Return the estimated trip count from metadata unless the metadata is
-  // missing or has no value.
+  // missing or has no value.  Return std::nullopt if it's zero.
   if (auto TC = getOptionalIntLoopAttribute(L, LLVMLoopEstimatedTripCount)) {
-    assert(TC != 0 && "Reached loop header executes at least one iteration");
     LLVM_DEBUG(dbgs() << "getLoopEstimatedTripCount: "
                       << LLVMLoopEstimatedTripCount << " metadata has trip "
-                      << "count of " << *TC << " for " << DbgLoop(L) << "\n");
-    return TC;
+                      << "count of " << *TC
+                      << (*TC == 0 ? " (returning std::nullopt)" : "")
+                      << " for " << DbgLoop(L) << "\n");
+    return *TC == 0 ? std::nullopt : std::optional(*TC);
   }
 
   // Estimate the trip count from latch branch weights.
@@ -949,13 +938,8 @@ bool llvm::setLoopEstimatedTripCount(
   if (!LatchBranch)
     return false;
 
-  // Set the metadata.  Some users of the estimated trip count rely on the value
-  // to be non-zero.
-  if (!EstimatedloopInvocationWeight) {
-    setLoopStringMetadata(L, LLVMLoopEstimatedTripCount,
-                          EstimatedTripCount == 0 ? 1 : EstimatedTripCount);
-    return true;
-  }
+  // Set the metadata.
+  addStringMetadataToLoop(L, LLVMLoopEstimatedTripCount, EstimatedTripCount);
 
   // At the moment, we currently support changing the estimated trip count in
   // the latch branch's branch weights only.  We could extend this API to
@@ -964,13 +948,8 @@ bool llvm::setLoopEstimatedTripCount(
   // TODO: Eventually, once all passes have migrated away from setting branch
   // weights to indicate estimated trip counts, we will not set branch weights
   // here at all.
-
-  // Set the metadata.  Some users of the estimated trip count rely on the value
-  // to be non-zero.
-  setLoopStringMetadata(L, LLVMLoopEstimatedTripCount,
-                        EstimatedTripCount == 0
-                            ? std::nullopt
-                            : std::optional(EstimatedTripCount));
+  if (!EstimatedloopInvocationWeight)
+    return true;
 
   // Calculate taken and exit weights.
   unsigned LatchExitWeight = 0;

llvm/test/Transforms/LoopUnroll/branch-weights-freq/unroll-epilog.ll

@@ -111,8 +111,7 @@ do.end:
 !0 = !{!"branch_weights", i32 1, i32 10}
 
 ; ------------------------------------------------------------------------------
-; Check branch weight metadata and estimated trip count metadata.  The minimum
-; valid estimated trip count is 1 not 0.
+; Check branch weight metadata and estimated trip count metadata.
 ;
 ;  UR2: ![[#PROF_UR_GUARD]] = !{!"branch_weights", i32 195225786, i32 1952257862}
 ;  UR4: ![[#PROF_UR_GUARD]] = !{!"branch_weights", i32 534047398, i32 1613436250}
@@ -136,7 +135,7 @@ do.end:
 ;  UR4: ![[#LOOP_UR_TC]] = !{!"llvm.loop.estimated_trip_count", i32 2}
 ; UR10: ![[#LOOP_UR_TC]] = !{!"llvm.loop.estimated_trip_count", i32 1}
 ; UR11: ![[#LOOP_UR_TC]] = !{!"llvm.loop.estimated_trip_count", i32 1}
-; UR12: ![[#LOOP_UR_TC]] = !{!"llvm.loop.estimated_trip_count", i32 1}
+; UR12: ![[#LOOP_UR_TC]] = !{!"llvm.loop.estimated_trip_count", i32 0}
 ;   UR: ![[#DISABLE]] = !{!"llvm.loop.unroll.disable"}
 ;
 ;  UR2: ![[#PROF_RM_GUARD]] = !{!"branch_weights", i32 1022611260, i32 1124872388}
@@ -152,9 +151,10 @@ do.end:
 
 ;  UR4: ![[#LOOP_RM_LATCH]] = distinct !{![[#LOOP_RM_LATCH]], ![[#LOOP_RM_TC:]], ![[#DISABLE:]]}
 ; UR10: ![[#LOOP_RM_LATCH]] = distinct !{![[#LOOP_RM_LATCH]], ![[#LOOP_UR_TC:]], ![[#DISABLE:]]}
-; UR11: ![[#LOOP_RM_LATCH]] = distinct !{![[#LOOP_RM_LATCH]], ![[#LOOP_UR_TC:]], ![[#DISABLE:]]}
+; UR11: ![[#LOOP_RM_LATCH]] = distinct !{![[#LOOP_RM_LATCH]], ![[#LOOP_RM_TC:]], ![[#DISABLE:]]}
 ; UR12: ![[#LOOP_RM_LATCH]] = distinct !{![[#LOOP_RM_LATCH]], ![[#LOOP_RM_TC:]], ![[#DISABLE:]]}
 ;
 ;  UR4: ![[#LOOP_RM_TC]] = !{!"llvm.loop.estimated_trip_count", i32 3}
-; For UR10 and UR11, llvm.loop.estimated_trip_count is the same for both loops.
+; For UR10, llvm.loop.estimated_trip_count is the same for both loops.
+; UR11: ![[#LOOP_RM_TC]] = !{!"llvm.loop.estimated_trip_count", i32 0}
 ; UR12: ![[#LOOP_RM_TC]] = !{!"llvm.loop.estimated_trip_count", i32 11}

llvm/test/Transforms/LoopUnroll/peel-loop-pgo-deopt.ll

@@ -86,5 +86,5 @@ attributes #1 = { nounwind optsize }
 ;CHECK: !16 = !{!"branch_weights", i32 3001, i32 1001}
 ;CHECK: !17 = distinct !{!17, !18, !19, {{.*}}}
 ;CHECK: !18 = !{!"llvm.loop.peeled.count", i32 4}
-;CHECK: !19 = !{!"llvm.loop.estimated_trip_count", i32 1}
+;CHECK: !19 = !{!"llvm.loop.estimated_trip_count", i32 0}
 

llvm/test/Transforms/LoopUnroll/peel-loop-pgo.ll

@@ -106,5 +106,5 @@ attributes #1 = { nounwind optsize }
 ;CHECK: !15 = !{!"branch_weights", i32 3001, i32 1001}
 ;CHECK: !16 = distinct !{!16, !17, !18, {{.*}}}
 ;CHECK: !17 = !{!"llvm.loop.peeled.count", i32 4}
-;CHECK: !18 = !{!"llvm.loop.estimated_trip_count", i32 1}
+;CHECK: !18 = !{!"llvm.loop.estimated_trip_count", i32 0}
 

llvm/test/Transforms/LoopUnroll/runtime-loop-branchweight.ll

@@ -74,8 +74,6 @@ for.end:
 ; 1073706403 / (1073706403 + 1073777245).
 ; CHECK: ![[#PROF_RM_LATCH]] = !{!"branch_weights", i32 1073706403, i32 1073777245}
 
-; 10000%4 = 0, so the probability of reaching the remainder loop header is low.
-; If it is reached, at least one iteration will execute.  The minimum valid
-; estimated trip count is 1.
+; 10000%4 = 0
 ; CHECK: ![[#LOOP_RM_LATCH]] = distinct !{![[#LOOP_RM_LATCH]], ![[#LOOP_RM_TC:]], ![[#DISABLE:]]}
-; CHECK: ![[#LOOP_RM_TC]] = !{!"llvm.loop.estimated_trip_count", i32 1}
+; CHECK: ![[#LOOP_RM_TC]] = !{!"llvm.loop.estimated_trip_count", i32 0}