[DenseMap] Fix constness issues with lookup_or #139247

artagnon · 2025-05-09T11:53:31Z

Also demonstrate its use in ScalarEvolution.

llvmbot · 2025-05-09T11:54:04Z

@llvm/pr-subscribers-llvm-analysis

Author: Ramkumar Ramachandra (artagnon)

Changes

Also demonstrate its use in ScalarEvolution.

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

3 Files Affected:

(modified) llvm/include/llvm/ADT/DenseMap.h (+7-2)
(modified) llvm/lib/Analysis/ScalarEvolution.cpp (+11-21)
(modified) llvm/unittests/ADT/DenseMapTest.cpp (+3-1)

diff --git a/llvm/include/llvm/ADT/DenseMap.h b/llvm/include/llvm/ADT/DenseMap.h
index 4df50e03de94b..acc81f069a5aa 100644
--- a/llvm/include/llvm/ADT/DenseMap.h
+++ b/llvm/include/llvm/ADT/DenseMap.h
@@ -220,8 +220,13 @@ class DenseMapBase : public DebugEpochBase {
   // Return the entry with the specified key, or \p Default. This variant is
   // useful, because `lookup` cannot be used with non-default-constructible
   // values.
-  ValueT lookup_or(const_arg_type_t<KeyT> Val,
-                   const_arg_type_t<ValueT> Default) const {
+  ValueT lookup_or(const_arg_type_t<KeyT> Val, const ValueT &Default) const {
+    if (const BucketT *Bucket = doFind(Val))
+      return Bucket->getSecond();
+    return Default;
+  }
+
+  ValueT lookup_or(const_arg_type_t<KeyT> Val, ValueT &&Default) const {
     if (const BucketT *Bucket = doFind(Val))
       return Bucket->getSecond();
     return Default;
diff --git a/llvm/lib/Analysis/ScalarEvolution.cpp b/llvm/lib/Analysis/ScalarEvolution.cpp
index 3f9614254ae7a..0b0748b7ffe78 100644
--- a/llvm/lib/Analysis/ScalarEvolution.cpp
+++ b/llvm/lib/Analysis/ScalarEvolution.cpp
@@ -9571,15 +9571,14 @@ getConstantEvolvingPHIOperands(Instruction *UseInst, const Loop *L,
     if (!OpInst || !canConstantEvolve(OpInst, L)) return nullptr;
 
     PHINode *P = dyn_cast<PHINode>(OpInst);
-    if (!P)
+    if (!P) {
       // If this operand is already visited, reuse the prior result.
       // We may have P != PHI if this is the deepest point at which the
       // inconsistent paths meet.
-      P = PHIMap.lookup(OpInst);
-    if (!P) {
       // Recurse and memoize the results, whether a phi is found or not.
       // This recursive call invalidates pointers into PHIMap.
-      P = getConstantEvolvingPHIOperands(OpInst, L, PHIMap, Depth + 1);
+      P = PHIMap.lookup_or(
+          OpInst, getConstantEvolvingPHIOperands(OpInst, L, PHIMap, Depth + 1));
       PHIMap[OpInst] = P;
     }
     if (!P)
@@ -15860,10 +15859,7 @@ const SCEV *ScalarEvolution::LoopGuards::rewrite(const SCEV *Expr) const {
     const SCEV *visitAddRecExpr(const SCEVAddRecExpr *Expr) { return Expr; }
 
     const SCEV *visitUnknown(const SCEVUnknown *Expr) {
-      auto I = Map.find(Expr);
-      if (I == Map.end())
-        return Expr;
-      return I->second;
+      return Map.lookup_or(Expr, Expr);
     }
 
     const SCEV *visitZeroExtendExpr(const SCEVZeroExtendExpr *Expr) {
@@ -15891,25 +15887,19 @@ const SCEV *ScalarEvolution::LoopGuards::rewrite(const SCEV *Expr) const {
     }
 
     const SCEV *visitSignExtendExpr(const SCEVSignExtendExpr *Expr) {
-      auto I = Map.find(Expr);
-      if (I == Map.end())
-        return SCEVRewriteVisitor<SCEVLoopGuardRewriter>::visitSignExtendExpr(
-            Expr);
-      return I->second;
+      return Map.lookup_or(
+          Expr,
+          SCEVRewriteVisitor<SCEVLoopGuardRewriter>::visitSignExtendExpr(Expr));
     }
 
     const SCEV *visitUMinExpr(const SCEVUMinExpr *Expr) {
-      auto I = Map.find(Expr);
-      if (I == Map.end())
-        return SCEVRewriteVisitor<SCEVLoopGuardRewriter>::visitUMinExpr(Expr);
-      return I->second;
+      return Map.lookup_or(
+          Expr, SCEVRewriteVisitor<SCEVLoopGuardRewriter>::visitUMinExpr(Expr));
     }
 
     const SCEV *visitSMinExpr(const SCEVSMinExpr *Expr) {
-      auto I = Map.find(Expr);
-      if (I == Map.end())
-        return SCEVRewriteVisitor<SCEVLoopGuardRewriter>::visitSMinExpr(Expr);
-      return I->second;
+      return Map.lookup_or(
+          Expr, SCEVRewriteVisitor<SCEVLoopGuardRewriter>::visitSMinExpr(Expr));
     }
 
     const SCEV *visitAddExpr(const SCEVAddExpr *Expr) {
diff --git a/llvm/unittests/ADT/DenseMapTest.cpp b/llvm/unittests/ADT/DenseMapTest.cpp
index d002a3c15cf52..cf90b10495a14 100644
--- a/llvm/unittests/ADT/DenseMapTest.cpp
+++ b/llvm/unittests/ADT/DenseMapTest.cpp
@@ -668,7 +668,9 @@ TEST(DenseMapCustomTest, LookupOr) {
 
   EXPECT_EQ(M.lookup_or(0, 4u), 3u);
   EXPECT_EQ(M.lookup_or(1, 4u), 0u);
-  EXPECT_EQ(M.lookup_or(2, 4u), 4u);
+
+  NonDefaultConstructible DefaultV = M.lookup_or(2, 4u);
+  EXPECT_EQ(DefaultV, 4u);
 }
 
 // Key traits that allows lookup with either an unsigned or char* key;

llvmbot · 2025-05-09T11:54:04Z

@llvm/pr-subscribers-llvm-adt

Author: Ramkumar Ramachandra (artagnon)

Changes

Also demonstrate its use in ScalarEvolution.

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

3 Files Affected:

(modified) llvm/include/llvm/ADT/DenseMap.h (+7-2)
(modified) llvm/lib/Analysis/ScalarEvolution.cpp (+11-21)
(modified) llvm/unittests/ADT/DenseMapTest.cpp (+3-1)

<!DOCTYPE html>
<html>
  <head>
    <meta http-equiv="Content-type" content="text/html; charset=utf-8">
    <meta http-equiv="Content-Security-Policy" content="default-src 'none'; base-uri 'self'; connect-src 'self'; form-action 'self'; img-src 'self' data:; script-src 'self'; style-src 'unsafe-inline'">
    <meta content="origin" name="referrer">
    <title>Rate limit &middot; GitHub</title>
    <meta name="viewport" content="width=device-width">
    <style type="text/css" media="screen">
      body {
        background-color: #f6f8fa;
        color: #24292e;
        font-family: -apple-system,BlinkMacSystemFont,Segoe UI,Helvetica,Arial,sans-serif,Apple Color Emoji,Segoe UI Emoji,Segoe UI Symbol;
        font-size: 14px;
        line-height: 1.5;
        margin: 0;
      }

      .container { margin: 50px auto; max-width: 600px; text-align: center; padding: 0 24px; }

      a { color: #0366d6; text-decoration: none; }
      a:hover { text-decoration: underline; }

      h1 { line-height: 60px; font-size: 48px; font-weight: 300; margin: 0px; text-shadow: 0 1px 0 #fff; }
      p { color: rgba(0, 0, 0, 0.5); margin: 20px 0 40px; }

      ul { list-style: none; margin: 25px 0; padding: 0; }
      li { display: table-cell; font-weight: bold; width: 1%; }

      .logo { display: inline-block; margin-top: 35px; }
      .logo-img-2x { display: none; }
      @media
      only screen and (-webkit-min-device-pixel-ratio: 2),
      only screen and (   min--moz-device-pixel-ratio: 2),
      only screen and (     -o-min-device-pixel-ratio: 2/1),
      only screen and (        min-device-pixel-ratio: 2),
      only screen and (                min-resolution: 192dpi),
      only screen and (                min-resolution: 2dppx) {
        .logo-img-1x { display: none; }
        .logo-img-2x { display: inline-block; }
      }

      #suggestions {
        margin-top: 35px;
        color: #ccc;
      }
      #suggestions a {
        color: #666666;
        font-weight: 200;
        font-size: 14px;
        margin: 0 10px;
      }

    </style>
  </head>
  <body>

    <div class="container">

      <h1>Whoa there!</h1>
      <p>You have exceeded a secondary rate limit.<br><br>
        Please wait a few minutes before you try again;<br>
        in some cases this may take up to an hour.
      </p>
      <div id="suggestions">
        <a href="https://support.github.com/contact">Contact Support</a> &mdash;
        <a href="https://githubstatus.com">GitHub Status</a> &mdash;
        <a href="https://twitter.com/githubstatus">@githubstatus</a>
      </div>

      <a href="/" class="logo logo-img-1x">
        <img width="32" height="32" title="" alt="" src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAYAAABzenr0AAAAGXRFWHRTb2Z0d2FyZQBBZG9iZSBJbWFnZVJlYWR5ccllPAAAAyRpVFh0WE1MOmNvbS5hZG9iZS54bXAAAAAAADw/eHBhY2tldCBiZWdpbj0i77u/IiBpZD0iVzVNME1wQ2VoaUh6cmVTek5UY3prYzlkIj8+IDx4OnhtcG1ldGEgeG1sbnM6eD0iYWRvYmU6bnM6bWV0YS8iIHg6eG1wdGs9IkFkb2JlIFhNUCBDb3JlIDUuMy1jMDExIDY2LjE0NTY2MSwgMjAxMi8wMi8wNi0xNDo1NjoyNyAgICAgICAgIj4gPHJkZjpSREYgeG1sbnM6cmRmPSJodHRwOi8vd3d3LnczLm9yZy8xOTk5LzAyLzIyLXJkZi1zeW50YXgtbnMjIj4gPHJkZjpEZXNjcmlwdGlvbiByZGY6YWJvdXQ9IiIgeG1sbnM6eG1wPSJodHRwOi8vbnMuYWRvYmUuY29tL3hhcC8xLjAvIiB4bWxuczp4bXBNTT0iaHR0cDovL25zLmFkb2JlLmNvbS94YXAvMS4wL21tLyIgeG1sbnM6c3RSZWY9Imh0dHA6Ly9ucy5hZG9iZS5jb20veGFwLzEuMC9zVHlwZS9SZXNvdXJjZVJlZiMiIHhtcDpDcmVhdG9yVG9vbD0iQWRvYmUgUGhvdG9zaG9wIENTNiAoTWFjaW50b3NoKSIgeG1wTU06SW5zdGFuY2VJRD0ieG1wLmlpZDpFMTZCRDY3REIzRjAxMUUyQUQzREIxQzRENUFFNUM5NiIgeG1wTU06RG9jdW1lbnRJRD0ieG1wLmRpZDpFMTZCRDY3RUIzRjAxMUUyQUQzREIxQzRENUFFNUM5NiI+IDx4bXBNTTpEZXJpdmVkRnJvbSBzdFJlZjppbnN0YW5jZUlEPSJ4bXAuaWlkOkUxNkJENjdCQjNGMDExRTJBRDNEQjFDNEQ1QUU1Qzk2IiBzdFJlZjpkb2N1bWVudElEPSJ4bXAuZGlkOkUxNkJENjdDQjNGMDExRTJBRDNEQjFDNEQ1QUU1Qzk2Ii8+IDwvcmRmOkRlc2NyaXB0aW9uPiA8L3JkZjpSREY+IDwveDp4bXBtZXRhPiA8P3hwYWNrZXQgZW5kPSJyIj8+SM9MCAAAA+5JREFUeNrEV11Ik1EY3s4+ddOp29Q5b0opCgKFsoKoi5Kg6CIhuwi6zLJLoYLopq4qsKKgi4i6CYIoU/q5iDAKs6syoS76IRWtyJ+p7cdt7sf1PGOD+e0c3dygAx/67ZzzPM95/877GYdHRg3ZjMXFxepQKNS6sLCwJxqNNuFpiMfjVs4ZjUa/pmmjeD6VlJS8NpvNT4QQ7mxwjSsJiEQim/1+/9lgMHgIr5ohuxG1WCw9Vqv1clFR0dCqBODElV6v90ogEDjGdYbVjXhpaendioqK07CIR7ZAqE49PT09BPL2PMgTByQGsYiZlQD4uMXtdr+JxWINhgINYhGT2MsKgMrm2dnZXgRXhaHAg5jEJodUAHxux4LudHJE9RdEdA+i3Juz7bGHe4mhE9FNrgwBCLirMFV9Okh5eflFh8PR5nK5nDabrR2BNJlKO0T35+Li4n4+/J+/JQCxhmu5h3uJoXNHPbmWZAHMshWB8l5/ipqammaAf0zPDDx1ONV3vurdidqwAQL+pEc8sLcAe1CCvQ3YHxIW8Pl85xSWNC1hADDIv0rIE/o4J0k3kww4xSlwIhcq3EFFOm7KN/hUGOQkt0CFa5WpNJlMvxBEz/IVQAxg/ZRZl9wiHA63yDYieM7DnLP5CiAGsC7I5sgtYKJGWe2A8seFqgFJrJjEPY1Cn3pJ8/9W1e5VWsFDTEmFrBcoDhZJEQkXuhICMyKpjhahqN21hRYATKfUOlDmkygrR4o4C0VOLGJKrOITKB4jijzdXygBKixyC5TDQdnk/Pz8qRw6oOWGlsTKGOQW6OH6FBWsyePxdOXLTgxiyebILZCjz+GLgMIKnXNzc49YMlcRdHXcSwxFVgTInQhC9G33UhNoJLuqq6t345p9y3eUy8OTk5PjAHuI9uo4b07FBaOhsu0A4Unc+T1TU1Nj3KsSSE5yJ65jqF2DDd8QqWYmAZrIM2VlZTdnZmb6AbpdV9V6ec9znf5Q7HjYumdRE0JOp3MjitO4SFa+cZz8Umqe3TCbSLvdfkR/kWDdNQl5InuTcysOcpFT35ZrbBxx4p3JAHlZVVW1D/634VRt+FvLBgK/v5LV9WS+10xMTEwtRw7XvqOL+e2Q8V3AYIOIAXQ26/heWVnZCVfcyKHg2CBgTpmPmjYM8l24GyaUHyaIh7XwfR9ErE8qHoDfn2LTNAVC0HX6MFcBIP8Bi+6F6cdW/DICkANRfx99fEYFQ7Nph5i/uQiA214gno7K+guhaiKg9gC62+M8eR7XsBsYJ4ilam60Fb7r7uAj8wFyuwM1oIOWgfmDy6RXEEQzJMPe23DXrVS7rtyD3Df8z/FPgAEAzWU5Ku59ZAUAAAAASUVORK5CYII=">
      </a>

      <a href="/" class="logo logo-img-2x">
        <img width="32" height="32" title="" alt="" src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAEAAAABACAYAAACqaXHeAAAAGXRFWHRTb2Z0d2FyZQBBZG9iZSBJbWFnZVJlYWR5ccllPAAAAyRpVFh0WE1MOmNvbS5hZG9iZS54bXAAAAAAADw/eHBhY2tldCBiZWdpbj0i77u/IiBpZD0iVzVNME1wQ2VoaUh6cmVTek5UY3prYzlkIj8+IDx4OnhtcG1ldGEgeG1sbnM6eD0iYWRvYmU6bnM6bWV0YS8iIHg6eG1wdGs9IkFkb2JlIFhNUCBDb3JlIDUuMy1jMDExIDY2LjE0NTY2MSwgMjAxMi8wMi8wNi0xNDo1NjoyNyAgICAgICAgIj4gPHJkZjpSREYgeG1sbnM6cmRmPSJodHRwOi8vd3d3LnczLm9yZy8xOTk5LzAyLzIyLXJkZi1zeW50YXgtbnMjIj4gPHJkZjpEZXNjcmlwdGlvbiByZGY6YWJvdXQ9IiIgeG1sbnM6eG1wPSJodHRwOi8vbnMuYWRvYmUuY29tL3hhcC8xLjAvIiB4bWxuczp4bXBNTT0iaHR0cDovL25zLmFkb2JlLmNvbS94YXAvMS4wL21tLyIgeG1sbnM6c3RSZWY9Imh0dHA6Ly9ucy5hZG9iZS5jb20veGFwLzEuMC9zVHlwZS9SZXNvdXJjZVJlZiMiIHhtcDpDcmVhdG9yVG9vbD0iQWRvYmUgUGhvdG9zaG9wIENTNiAoTWFjaW50b3NoKSIgeG1wTU06SW5zdGFuY2VJRD0ieG1wLmlpZDpEQUM1QkUxRUI0MUMxMUUyQUQzREIxQzRENUFFNUM5NiIgeG1wTU06RG9jdW1lbnRJRD0ieG1wLmRpZDpEQUM1QkUxRkI0MUMxMUUyQUQzREIxQzRENUFFNUM5NiI+IDx4bXBNTTpEZXJpdmVkRnJvbSBzdFJlZjppbnN0YW5jZUlEPSJ4bXAuaWlkOkUxNkJENjdGQjNGMDExRTJBRDNEQjFDNEQ1QUU1Qzk2IiBzdFJlZjpkb2N1bWVudElEPSJ4bXAuZGlkOkUxNkJENjgwQjNGMDExRTJBRDNEQjFDNEQ1QUU1Qzk2Ii8+IDwvcmRmOkRlc2NyaXB0aW9uPiA8L3JkZjpSREY+IDwveDp4bXBtZXRhPiA8P3hwYWNrZXQgZW5kPSJyIj8+hfPRaQAAB6lJREFUeNrsW2mME2UYbodtt+2222u35QheoCCYGBQligIJgkZJNPzgigoaTEj8AdFEMfADfyABkgWiiWcieK4S+QOiHAYUj2hMNKgYlEujpNttu9vttbvdw+chU1K6M535pt3ubHCSyezR+b73eb73+t7vrfXsufOW4bz6+vom9/b23ovnNNw34b5xYGAgODg46Mbt4mesVmsWd1qSpHhdXd2fuP/Afcput5/A88xwymcdBgLqenp6FuRyuWV4zu/v759QyWBjxoz5t76+/gun09mK5xFyakoCAPSaTCazNpvNPoYVbh6O1YKGRF0u13sNDQ27QMzfpiAAKj0lnU6/gBVfAZW2WWpwwVzy0IgP3G73FpjI6REhAGA9qVRqA1b9mVoBVyIC2tDi8Xg24+dUzQiAbS/s7Ox8G2o/3mKCC+Zw0efzPQEfcVjYrARX3dbV1bUtHo8fMgt42f+Mp0yUTVQbdWsAHVsikdiHkHaPxcQXQufXgUBgMRxme9U0AAxfH4vFvjM7eF6UkbJS5qoQwEQGA57Ac5JllFyUVZZ5ckUEgMVxsK2jlSYzI+QXJsiyjzNEAJyJAzb/KQa41jJKL8pODMQiTEAymXw5n8/P0IjD3bh7Rgog59aanxiIRTVvV/oj0tnHca/WMrVwODwB3raTGxzkBg/gnZVapFV62Wy2n5AO70HM/5wbJ0QnXyQSaVPDIuNZzY0V3ntHMwxiwHA0Gj2Np7ecIBDgaDAYXKCQJM1DhrgJ3nhulcPbl8j4NmHe46X/g60fwbz3aewjkqFQaAqebWU1AOqyQwt8Id6qEHMc97zu7u7FGGsn7HAiVuosVw7P35C1nccdgSCxop1dHeZswmfHMnxBo6ZTk+jN8dl/vF7vWofDsa+MLN9oEUBMxOb3+1eoEsBVw6Zmua49r8YmhAKDiEPcMwBsxMiqQ+ixzPFxZyqRpXARG/YOr1ObFJ0gUskXBbamcR1OKmMUvDxHRAu8/LmY3jFLMUpFqz9HxG65smYJdyKyECOxDiEAe/p1gjF2oonivZAsxVgl2daa4EQWCW6J55qFAFFZiJWYLxNQy2qOSUzGRsyXCUDIeliwAHEO4WSlWQBRFoZakXcKmCXmyXAKs0Ve9vl8q42WoIYpJU4hV3hKcNs8m9gl7p/xQ73eF5kB4j5mNrWmTJRNwAzqiV1CxjVTZCIkEq+Z1bZFZSN2CenmVAFVy4Plz8xKAGWjjAKFk6lCBMDR/MJjLLMSQNm43xAiQKTaA+9/wewhDjL+JVI1kkTSSOTcKbMTwPqESAot6dn6Fr1gHwVJju6IRuyiByPuUUBAg5DGkAgBmxlvdgIEK9gDkohdY/BJo4CAG0R8miRSsGABkgVQs4KXu098IgUXSSRsFAoKZiVAVDY2WUiiPTjYRi41KwGisrGsLtlsth8Fiwnz2fBkQvWfRtlE3iF2yW63/yCacXZ1dW02GwGyTFaRd4idJnCKHRaCxYRHoG5LTKT6SyiToP1fJHbmAYPYRR0UnZQtMnA6s0zg+GZBlt0Gdo7EPHgpE3Q6nZ8YyLhc8Xj8MJh/aKTAY+5FPAKHLE7RdwuYJZmNwzyCMkBCYyKROJBMJl9B/PXXCjjmCmDOVzH3fiPpObEWGqoKe4EBl8v1hlqsdLvd23mkxHM9pc9kMpmno9HoeTii7ewbHEZPPx1ztLS1tV3AnGuMjiNjvbQFuHw6zDo5By7dTPAQNBgMLrRarTkSls1mnwT7uwp9virx9QzbW/HuV/j5d/b+6jniKlllP8lkeONJDk+dq9GsQTnC4fB1heO0K47Hwe7WdDr9nAKgXwOBwHI+C45Htj1d6sd429TUNEcmUdc+PRaLHcvn87dXW4ugzdsaGxufL94NFv9zi1J7GVbhlvb2dnaJ3SVrxfc+n2+NTsZ7/H7/Mr3g5XdSIHyJSH1PZ+7fToyl2+ErqilgZ4NaLYB9goVGaHjR93Hv1ZrU4XDsFT20kH3PObzbWk0CgG1jacVIUnAQb9F+VexyLMzkpcLv0IJV7AHQIOCAUYHx7v5qgScmYHtTqSAyZLEJTK22Bie4iq3xsqpm4SAf9Hq9a2DnJ4uLK3SEULcdRvp3i3zHySqpficxEdsQc1NrlYXXvR+O7qASSezXB+h1SuUomgg9LL8BUoV4749EIolKh+EiqWmqVEZlDgHks2pxHw7xTqUQw9J5NcAXOK10AGIoZ6Zli6JY6Z1Q461KoZ4NiKLHarW+KDsxlDUPHZ5zPQZqUVDPJsTqb5n9malbpAh8C2XXDLl62+WZIDFRUlNVOiwencnNU3aQEkL+cDMSoLvZo2fQB7AJssNAuFuvorlDVVkkg2I87+jo2K2QAVphDrfyViK5VqtO34OkaxXCp+7drdDBCAdubm6eidX+2WwqT5komwh4YQLk+H4aE93h8Xg2gvHekQZOGSgLZTLyDTLJ4Lx9/KZWKBSainT4Iy3FqQBfnUZR42PKQFksBr9QKVXCPusD3OiA/RkQ5kP8qV/Jl1WywAp/6+dcmPM2zL1UrUahe4JqfnWWKXIul3uUbfP8njAFLW1OFr3gdFtZ72cNH+PtQT7/brW+NXqJAHh0y9V8/U/A1U7AfwIMAD7mS3pCbuWJAAAAAElFTkSuQmCC">
      </a>
    </div>
  </body>
</html>

llvm/lib/Analysis/ScalarEvolution.cpp

artagnon · 2025-05-15T10:17:37Z

Gentle ping. The const_arg_t<ValueT> is incorrect when the value in the map is non-cost. This patch isn't a blocker for anything yet, but it would be nice to get this fixed.

artagnon · 2025-05-27T18:35:17Z

Gentle ping.

kazutakahirata · 2025-05-27T19:39:03Z

@artagnon Do we need the const ValueT &Default variant? I'm looking at std::optional<T>::value_or as a reference, and it only has the rvalue reference variant.

artagnon · 2025-05-27T20:29:10Z

@artagnon Do we need the const ValueT &Default variant? I'm looking at std::optional<T>::value_or as a reference, and it only has the rvalue reference variant.

Good catch! I think that, by the same reasoning, several of the const-lvalue-reference variants can also be removed in a follow-up.

kazutakahirata · 2025-05-27T21:24:43Z

@artagnon Is there any way you can come with a test case that would fail without your change to DenseMap.h? I just tried your changes without DenseMap.h, and both ScalarEvolution.cpp and DenseMapTest.cpp were happy. This means that if somebody reverted your DenseMap.h change in the future, nothing would break.

By the way, what is the core issue here? const_arg_type_t<ValueT> prevents move semantics? Or am I missing something?

Also demonstrate its use in ScalarEvolution.

artagnon · 2025-05-28T09:29:02Z

@artagnon Is there any way you can come with a test case that would fail without your change to DenseMap.h? I just tried your changes without DenseMap.h, and both ScalarEvolution.cpp and DenseMapTest.cpp were happy. This means that if somebody reverted your DenseMap.h change in the future, nothing would break.

By the way, what is the core issue here? const_arg_type_t<ValueT> prevents move semantics? Or am I missing something?

Sorry about the thinko in the test case: should be fixed now. The included test case now fails with:

/Users/artagnon/src/llvm/llvm/include/llvm/ADT/DenseMap.h:227:12: error: cannot initialize return object of type 'unsigned int *' with an lvalue of type 'const_arg_type_t<unsigned int *>' (aka 'const unsigned int *')
  227 |     return Default;
      |            ^~~~~~~
/Users/artagnon/src/llvm/llvm/unittests/ADT/DenseMapTest.cpp:677:21: note: in instantiation of member function 'llvm::DenseMapBase<llvm::DenseMap<int, unsigned int *>, int, unsigned int *, llvm::DenseMapInfo<int>, llvm::detail::DenseMapPair<int, unsigned int *>>::lookup_or' requested here
  677 |   unsigned *Ret = M.lookup_or(0, &Default);
      |                     ^

without the DenseMap change. The issue isn't move semantics, but rather a constness.

kazutakahirata · 2025-05-28T18:06:22Z

Sorry about the thinko in the test case: should be fixed now. The included test case now fails with:

/Users/artagnon/src/llvm/llvm/include/llvm/ADT/DenseMap.h:227:12: error: cannot initialize return object of type 'unsigned int *' with an lvalue of type 'const_arg_type_t<unsigned int *>' (aka 'const unsigned int *')
  227 |     return Default;
      |            ^~~~~~~
/Users/artagnon/src/llvm/llvm/unittests/ADT/DenseMapTest.cpp:677:21: note: in instantiation of member function 'llvm::DenseMapBase<llvm::DenseMap<int, unsigned int *>, int, unsigned int *, llvm::DenseMapInfo<int>, llvm::detail::DenseMapPair<int, unsigned int *>>::lookup_or' requested here
  677 |   unsigned *Ret = M.lookup_or(0, &Default);
      |                     ^

without the DenseMap change. The issue isn't move semantics, but rather a constness.

I've verified the same failure.

So, the issue is about unnecessarily putting const on the value type. Thanks for the update and explanation!

kazutakahirata

LGTM. Thanks!

aaronpuchert · 2025-05-30T13:50:26Z

Could this be related to the build failure in https://lab.llvm.org/buildbot/#/builders/63/builds/6559?

FAILED: lib/Analysis/CMakeFiles/LLVMAnalysis.dir/ScalarEvolution.cpp.obj 
C:\PROGRA~2\MICROS~3\2019\PROFES~1\VC\Tools\MSVC\1428~1.293\bin\Hostx64\x64\cl.exe  /nologo /TP -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -IC:\b\slave\clang-x64-windows-msvc\build\stage1\lib\Analysis -IC:\b\slave\clang-x64-windows-msvc\llvm-project\llvm\lib\Analysis -IC:\b\slave\clang-x64-windows-msvc\build\stage1\include -IC:\b\slave\clang-x64-windows-msvc\llvm-project\llvm\include /DWIN32 /D_WINDOWS   /Zc:inline /Zc:preprocessor /Zc:__cplusplus /Oi /bigobj /permissive- /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd5105 -wd4324 -wd4251 -wd4275 -w14062 -we4238 /Gw /O2 /Ob2 /DNDEBUG -std:c++17 -MD  /EHs-c- /GR- /showIncludes /Folib\Analysis\CMakeFiles\LLVMAnalysis.dir\ScalarEvolution.cpp.obj /Fdlib\Analysis\CMakeFiles\LLVMAnalysis.dir\LLVMAnalysis.pdb /FS -c C:\b\slave\clang-x64-windows-msvc\llvm-project\llvm\lib\Analysis\ScalarEvolution.cpp
C:\b\slave\clang-x64-windows-msvc\llvm-project\llvm\lib\Analysis\ScalarEvolution.cpp(15907): error C2664: 'ValueT llvm::DenseMapBase<llvm::DenseMap<const llvm::SCEV *,ValueT,llvm::DenseMapInfo<KeyT,void>,llvm::detail::DenseMapPair<KeyT,ValueT>>,KeyT,ValueT,KeyInfoT,BucketT>::lookup_or(const llvm::SCEV *,ValueT &&) const': cannot convert argument 2 from 'const llvm::SCEVUnknown *' to 'ValueT &&'
        with
        [
            ValueT=const llvm::SCEV *,
            KeyT=const llvm::SCEV *,
            KeyInfoT=llvm::DenseMapInfo<const llvm::SCEV *,void>,
            BucketT=llvm::detail::DenseMapPair<const llvm::SCEV *,const llvm::SCEV *>
        ]
        and
        [
            ValueT=const llvm::SCEV *
        ]
C:\b\slave\clang-x64-windows-msvc\llvm-project\llvm\lib\Analysis\ScalarEvolution.cpp(15907): note: You cannot bind an lvalue to an rvalue reference
C:\b\slave\clang-x64-windows-msvc\llvm-project\llvm\include\llvm/ADT/DenseMap.h(223): note: see declaration of 'llvm::DenseMapBase<llvm::DenseMap<const llvm::SCEV *,const llvm::SCEV *,llvm::DenseMapInfo<KeyT,void>,llvm::detail::DenseMapPair<KeyT,ValueT>>,KeyT,ValueT,KeyInfoT,BucketT>::lookup_or'
        with
        [
            KeyT=const llvm::SCEV *,
            ValueT=const llvm::SCEV *,
            KeyInfoT=llvm::DenseMapInfo<const llvm::SCEV *,void>,
            BucketT=llvm::detail::DenseMapPair<const llvm::SCEV *,const llvm::SCEV *>
        ]

aaronpuchert · 2025-05-30T22:33:40Z

Side note: I think that MSVC is incorrect and the reference initialization should be allowed according to [dcl.init.ref]p5.4.2, because const SCEV* and const SCEVUnknown* are not reference-related.

artagnon · 2025-05-31T11:11:07Z

Side note: I think that MSVC is incorrect and the reference initialization should be allowed according to [dcl.init.ref]p5.4.2, because const SCEV* and const SCEVUnknown* are not reference-related.

Yes, there seems to be a bug in MSVC, and I think creating a const ValueT & variant would fix it? See #142268.

Also demonstrate its use in ScalarEvolution.

artagnon requested review from fhahn and dwblaikie May 9, 2025 11:53

artagnon requested a review from nikic as a code owner May 9, 2025 11:53

llvmbot added llvm:analysis llvm:adt labels May 9, 2025

nikic requested changes May 9, 2025

View reviewed changes

llvm/lib/Analysis/ScalarEvolution.cpp Outdated Show resolved Hide resolved

nikic reviewed May 9, 2025

View reviewed changes

llvm/lib/Analysis/ScalarEvolution.cpp Outdated Show resolved Hide resolved

artagnon force-pushed the densemap-lookupor-fix branch from d5ca963 to 9c22c18 Compare May 9, 2025 12:36

artagnon force-pushed the densemap-lookupor-fix branch from 9c22c18 to b9cd220 Compare May 27, 2025 20:28

artagnon and others added 2 commits May 28, 2025 11:02

[DenseMap] Fix constness issues with lookup_or

c1d2731

Also demonstrate its use in ScalarEvolution.

[DenseMapTest] Fix test

9f49a22

artagnon force-pushed the densemap-lookupor-fix branch from b9cd220 to 9f49a22 Compare May 28, 2025 09:27

kazutakahirata approved these changes May 28, 2025

View reviewed changes

artagnon merged commit 4bf67cd into llvm:main May 28, 2025
11 checks passed

artagnon deleted the densemap-lookupor-fix branch May 28, 2025 18:32

sivan-shani pushed a commit to sivan-shani/llvm-project that referenced this pull request Jun 3, 2025

[DenseMap] Fix constness issues with lookup_or (llvm#139247)

7a0a38c

Also demonstrate its use in ScalarEvolution.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[DenseMap] Fix constness issues with lookup_or #139247

[DenseMap] Fix constness issues with lookup_or #139247

Uh oh!

artagnon commented May 9, 2025

Uh oh!

llvmbot commented May 9, 2025

Uh oh!

llvmbot commented May 9, 2025

Uh oh!

Uh oh!

Uh oh!

artagnon commented May 15, 2025

Uh oh!

artagnon commented May 27, 2025

Uh oh!

kazutakahirata commented May 27, 2025

Uh oh!

artagnon commented May 27, 2025

Uh oh!

kazutakahirata commented May 27, 2025

Uh oh!

artagnon commented May 28, 2025

Uh oh!

kazutakahirata commented May 28, 2025

Uh oh!

kazutakahirata left a comment

Uh oh!

Uh oh!

aaronpuchert commented May 30, 2025

Uh oh!

aaronpuchert commented May 30, 2025

Uh oh!

artagnon commented May 31, 2025 •

edited

Loading

Uh oh!

Uh oh!

[DenseMap] Fix constness issues with lookup_or #139247

[DenseMap] Fix constness issues with lookup_or #139247

Uh oh!

Conversation

artagnon commented May 9, 2025

Uh oh!

llvmbot commented May 9, 2025

Uh oh!

llvmbot commented May 9, 2025

Uh oh!

Uh oh!

Uh oh!

artagnon commented May 15, 2025

Uh oh!

artagnon commented May 27, 2025

Uh oh!

kazutakahirata commented May 27, 2025

Uh oh!

artagnon commented May 27, 2025

Uh oh!

kazutakahirata commented May 27, 2025

Uh oh!

artagnon commented May 28, 2025

Uh oh!

kazutakahirata commented May 28, 2025

Uh oh!

kazutakahirata left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aaronpuchert commented May 30, 2025

Uh oh!

aaronpuchert commented May 30, 2025

Uh oh!

artagnon commented May 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

artagnon commented May 31, 2025 •

edited

Loading