[clang][Sema] Cleanup and optimize DiagnoseAssignmentEnum

[tbaederr]

Reorder the precondition checks to move the costly onces last. Also, only evaluate the RHS once to get the integral value.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

Reorder the precondition checks to move the costly onces last. Also, only evaluate the RHS once to get the integral value.

---

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

1 Files Affected:

• (modified) clang/lib/Sema/SemaStmt.cpp (+53-45)

diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index ed070649dc1f9..08f77b2e246e9 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -1741,57 +1741,65 @@ Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, Stmt *Switch,
 void
 Sema::DiagnoseAssignmentEnum(QualType DstType, QualType SrcType,
                              Expr *SrcExpr) {
+
+  const auto *ET = DstType->getAs<EnumType>();
+  if (!ET)
+    return;
+
+  if (SrcType->isIntegerType() ||
+      Context.hasSameUnqualifiedType(SrcType, DstType))
+    return;
+
+  if (SrcExpr->isTypeDependent() || SrcExpr->isValueDependent())
+    return;
+
+  const EnumDecl *ED = ET->getDecl();
+  if (!ED->isClosed())
+    return;
+
   if (Diags.isIgnored(diag::warn_not_in_enum_assignment, SrcExpr->getExprLoc()))
     return;
 
-  if (const EnumType *ET = DstType->getAs<EnumType>())
-    if (!Context.hasSameUnqualifiedType(SrcType, DstType) &&
-        SrcType->isIntegerType()) {
-      if (!SrcExpr->isTypeDependent() && !SrcExpr->isValueDependent() &&
-          SrcExpr->isIntegerConstantExpr(Context)) {
-        // Get the bitwidth of the enum value before promotions.
-        unsigned DstWidth = Context.getIntWidth(DstType);
-        bool DstIsSigned = DstType->isSignedIntegerOrEnumerationType();
+  std::optional<llvm::APSInt> RHSVal = SrcExpr->getIntegerConstantExpr(Context);
+  if (!RHSVal)
+    return;
 
-        llvm::APSInt RhsVal = SrcExpr->EvaluateKnownConstInt(Context);
-        AdjustAPSInt(RhsVal, DstWidth, DstIsSigned);
-        const EnumDecl *ED = ET->getDecl();
+  // Get the bitwidth of the enum value before promotions.
+  unsigned DstWidth = Context.getIntWidth(DstType);
+  bool DstIsSigned = DstType->isSignedIntegerOrEnumerationType();
+  AdjustAPSInt(*RHSVal, DstWidth, DstIsSigned);
 
-        if (!ED->isClosed())
-          return;
+  if (ED->hasAttr<FlagEnumAttr>()) {
+    if (!IsValueInFlagEnum(ED, *RHSVal, /*AllowMask=*/true))
+      Diag(SrcExpr->getExprLoc(), diag::warn_not_in_enum_assignment)
+          << DstType.getUnqualifiedType();
+    return;
+  }
 
-        if (ED->hasAttr<FlagEnumAttr>()) {
-          if (!IsValueInFlagEnum(ED, RhsVal, true))
-            Diag(SrcExpr->getExprLoc(), diag::warn_not_in_enum_assignment)
-              << DstType.getUnqualifiedType();
-        } else {
-          typedef SmallVector<std::pair<llvm::APSInt, EnumConstantDecl *>, 64>
-              EnumValsTy;
-          EnumValsTy EnumVals;
-
-          // Gather all enum values, set their type and sort them,
-          // allowing easier comparison with rhs constant.
-          for (auto *EDI : ED->enumerators()) {
-            llvm::APSInt Val = EDI->getInitVal();
-            AdjustAPSInt(Val, DstWidth, DstIsSigned);
-            EnumVals.push_back(std::make_pair(Val, EDI));
-          }
-          if (EnumVals.empty())
-            return;
-          llvm::stable_sort(EnumVals, CmpEnumVals);
-          EnumValsTy::iterator EIend = llvm::unique(EnumVals, EqEnumVals);
-
-          // See which values aren't in the enum.
-          EnumValsTy::const_iterator EI = EnumVals.begin();
-          while (EI != EIend && EI->first < RhsVal)
-            EI++;
-          if (EI == EIend || EI->first != RhsVal) {
-            Diag(SrcExpr->getExprLoc(), diag::warn_not_in_enum_assignment)
-                << DstType.getUnqualifiedType();
-          }
-        }
-      }
-    }
+  typedef SmallVector<std::pair<llvm::APSInt, EnumConstantDecl *>, 64>
+      EnumValsTy;
+  EnumValsTy EnumVals;
+
+  // Gather all enum values, set their type and sort them,
+  // allowing easier comparison with rhs constant.
+  for (auto *EDI : ED->enumerators()) {
+    llvm::APSInt Val = EDI->getInitVal();
+    AdjustAPSInt(Val, DstWidth, DstIsSigned);
+    EnumVals.emplace_back(Val, EDI);
+  }
+  if (EnumVals.empty())
+    return;
+  llvm::stable_sort(EnumVals, CmpEnumVals);
+  EnumValsTy::iterator EIend = llvm::unique(EnumVals, EqEnumVals);
+
+  // See which values aren't in the enum.
+  EnumValsTy::const_iterator EI = EnumVals.begin();
+  while (EI != EIend && EI->first < *RHSVal)
+    EI++;
+  if (EI == EIend || EI->first != *RHSVal) {
+    Diag(SrcExpr->getExprLoc(), diag::warn_not_in_enum_assignment)
+        << DstType.getUnqualifiedType();
+  }
 }
 
 StmtResult Sema::ActOnWhileStmt(SourceLocation WhileLoc,

[tbaederr]

http://llvm-compile-time-tracker.com/compare.php?from=843e362318e884991e517a54446b4faeacdad789&to=7b0a2b83146623a1674967ff4353a9eea6efd25e&stat=instructions:u

[Fznamznon]

That looks great imo

[shafik]

Very nice change.

How are you coming up w/ the target for your improvements? Is this something other folks could try and try and find some inspiration?

[shafik]

It would be interesting to test the case in which we have a enum and the enumerators values are out of order to verify the sorting actually works as expected e.g.

enum E {
  E1 = 100,
  E2 = 50,
  E3 = 75,
  E4 = 9,
  E5 = 99
};

I don't see a test that verifies that but I could be missing it.

[tbaederr]

How are you coming up w/ the target for your improvements? Is this something other folks could try and try and find some inspiration?

I ran the SemaExpr.cpp compilation command from the compile_commands.json through valgrind, load the profile in qcachegrind and see what doesn't make sense.