Address feedback from code review

* Use APInt for Divisor
* Move rotation normalization code into ExprConstShared.h

Author: chaitanyav

clang/lib/AST/ByteCode/InterpBuiltin.cpp

@@ -4145,41 +4145,7 @@ bool InterpretBuiltin(InterpState &S, CodePtr OpPC, const CallExpr *Call,
 
     return interp__builtin_elementwise_int_binop(
         S, OpPC, Call, [IsRotateRight](const APSInt &Value, APSInt Amount) {
-          // Normalize shift amount to [0, BitWidth) range to match runtime
-          // behavior. This matches the algorithm in ExprConstant.cpp.
-          unsigned BitWidth = Value.getBitWidth();
-          unsigned AmtBitWidth = Amount.getBitWidth();
-          if (BitWidth == 1) {
-            // Rotating a 1-bit value is always a no-op
-            Amount = APSInt(llvm::APInt(AmtBitWidth, 0), Amount.isUnsigned());
-          } else {
-            // Divisor is always unsigned to avoid misinterpreting BitWidth as
-            // negative in small bit widths (e.g., BitWidth=2 would be -2 if
-            // signed).
-            APSInt Divisor;
-            if (AmtBitWidth > BitWidth) {
-              Divisor = APSInt(llvm::APInt(AmtBitWidth, BitWidth),
-                               /*isUnsigned=*/true);
-            } else {
-              Divisor =
-                  APSInt(llvm::APInt(BitWidth, BitWidth), /*isUnsigned=*/true);
-              if (AmtBitWidth < BitWidth) {
-                Amount = Amount.extend(BitWidth);
-              }
-            }
-
-            // Normalize to [0, BitWidth)
-            if (Amount.isSigned()) {
-              Amount = APSInt(Amount.srem(Divisor), /*isUnsigned=*/false);
-              if (Amount.isNegative()) {
-                APSInt SignedDivisor(Divisor, /*isUnsigned=*/false);
-                Amount += SignedDivisor;
-              }
-            } else {
-              Amount = APSInt(Amount.urem(Divisor), /*isUnsigned=*/true);
-            }
-          }
-
+          Amount = NormalizeRotateAmount(Value, Amount);
           return IsRotateRight ? Value.rotr(Amount.getZExtValue())
                                : Value.rotl(Amount.getZExtValue());
         });

clang/lib/AST/ExprConstShared.h

@@ -19,8 +19,8 @@
 
 namespace llvm {
 class APFloat;
-class APInt;
 class APSInt;
+class APInt;
 }
 namespace clang {
 class QualType;
@@ -81,5 +81,7 @@ uint8_t GFNIMultiplicativeInverse(uint8_t Byte);
 uint8_t GFNIMul(uint8_t AByte, uint8_t BByte);
 uint8_t GFNIAffine(uint8_t XByte, const llvm::APInt &AQword,
                    const llvm::APSInt &Imm, bool Inverse = false);
+llvm::APSInt NormalizeRotateAmount(const llvm::APSInt &Value,
+                                   const llvm::APSInt &Amount);
 
 #endif

clang/lib/AST/ExprConstant.cpp

@@ -16352,42 +16352,12 @@ bool IntExprEvaluator::VisitBuiltinCallExpr(const CallExpr *E,
   case Builtin::BI_rotr:
   case Builtin::BI_lrotr:
   case Builtin::BI_rotr64: {
-    APSInt Val, Amt;
-    if (!EvaluateInteger(E->getArg(0), Val, Info) ||
-        !EvaluateInteger(E->getArg(1), Amt, Info))
+    APSInt Value, Amount;
+    if (!EvaluateInteger(E->getArg(0), Value, Info) ||
+        !EvaluateInteger(E->getArg(1), Amount, Info))
       return false;
 
-    // Normalize shift amount to [0, BitWidth) range to match runtime behavior
-    unsigned BitWidth = Val.getBitWidth();
-    unsigned AmtBitWidth = Amt.getBitWidth();
-    if (BitWidth == 1) {
-      // Rotating a 1-bit value is always a no-op
-      Amt = APSInt(APInt(AmtBitWidth, 0), Amt.isUnsigned());
-    } else {
-      // Divisor is always unsigned to avoid misinterpreting BitWidth as
-      // negative in small bit widths (e.g., BitWidth=2 would be -2 if signed).
-      APSInt Divisor;
-      if (AmtBitWidth > BitWidth) {
-        Divisor =
-            APSInt(llvm::APInt(AmtBitWidth, BitWidth), /*isUnsigned=*/true);
-      } else {
-        Divisor = APSInt(llvm::APInt(BitWidth, BitWidth), /*isUnsigned=*/true);
-        if (AmtBitWidth < BitWidth) {
-          Amt = Amt.extend(BitWidth);
-        }
-      }
-
-      // Normalize to [0, BitWidth)
-      if (Amt.isSigned()) {
-        Amt = APSInt(Amt.srem(Divisor), /*isUnsigned=*/false);
-        if (Amt.isNegative()) {
-          APSInt SignedDivisor(Divisor, /*isUnsigned=*/false);
-          Amt += SignedDivisor;
-        }
-      } else {
-        Amt = APSInt(Amt.urem(Divisor), /*isUnsigned=*/true);
-      }
-    }
+    Amount = NormalizeRotateAmount(Value, Amount);
 
     switch (BuiltinOp) {
     case Builtin::BI__builtin_rotateright8:
@@ -16400,9 +16370,11 @@ bool IntExprEvaluator::VisitBuiltinCallExpr(const CallExpr *E,
     case Builtin::BI_rotr:
     case Builtin::BI_lrotr:
     case Builtin::BI_rotr64:
-      return Success(APSInt(Val.rotr(Amt.getZExtValue()), Val.isUnsigned()), E);
+      return Success(
+          APSInt(Value.rotr(Amount.getZExtValue()), Value.isUnsigned()), E);
     default:
-      return Success(APSInt(Val.rotl(Amt.getZExtValue()), Val.isUnsigned()), E);
+      return Success(
+          APSInt(Value.rotl(Amount.getZExtValue()), Value.isUnsigned()), E);
     }
   }
 
@@ -19781,6 +19753,42 @@ void HandleComplexComplexDiv(APFloat A, APFloat B, APFloat C, APFloat D,
   }
 }
 
+APSInt NormalizeRotateAmount(const APSInt &Value, const APSInt &Amount) {
+  // Normalize shift amount to [0, BitWidth) range to match runtime behavior
+  APSInt NormAmt = Amount;
+  unsigned BitWidth = Value.getBitWidth();
+  unsigned AmtBitWidth = NormAmt.getBitWidth();
+  if (BitWidth == 1) {
+    // Rotating a 1-bit value is always a no-op
+    NormAmt = APSInt(APInt(AmtBitWidth, 0), NormAmt.isUnsigned());
+  } else {
+    // Divisor is always unsigned to avoid misinterpreting BitWidth as
+    // negative in small bit widths (e.g., BitWidth=2 would be -2 if signed).
+    APInt Divisor;
+    if (AmtBitWidth > BitWidth) {
+      Divisor = llvm::APInt(AmtBitWidth, BitWidth);
+    } else {
+      Divisor = llvm::APInt(BitWidth, BitWidth);
+      if (AmtBitWidth < BitWidth) {
+        NormAmt = NormAmt.extend(BitWidth);
+      }
+    }
+
+    // Normalize to [0, BitWidth)
+    if (NormAmt.isSigned()) {
+      NormAmt = APSInt(NormAmt.srem(Divisor), /*isUnsigned=*/false);
+      if (NormAmt.isNegative()) {
+        APSInt SignedDivisor(Divisor, /*isUnsigned=*/false);
+        NormAmt += SignedDivisor;
+      }
+    } else {
+      NormAmt = APSInt(NormAmt.urem(Divisor), /*isUnsigned=*/true);
+    }
+  }
+
+  return NormAmt;
+}
+
 bool ComplexExprEvaluator::VisitBinaryOperator(const BinaryOperator *E) {
   if (E->isPtrMemOp() || E->isAssignmentOp() || E->getOpcode() == BO_Comma)
     return ExprEvaluatorBaseTy::VisitBinaryOperator(E);