Address review feedback

- add createShift functions to CIRBuilder
- Rephrase ShiftOpn description comment
- Remove folding for now
- Always zero-extend instead of potentially sign-extend
- Tests with diffferently sized integral types

Author: mmha

clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h

@@ -353,6 +353,44 @@ class CIRBaseBuilderTy : public mlir::OpBuilder {
     return create<cir::CmpOp>(loc, getBoolTy(), kind, lhs, rhs);
   }
 
+  mlir::Value createShift(mlir::Location loc, mlir::Value lhs, mlir::Value rhs,
+                          bool isShiftLeft) {
+    return create<cir::ShiftOp>(loc, lhs.getType(), lhs, rhs, isShiftLeft);
+  }
+
+  mlir::Value createShift(mlir::Location loc, mlir::Value lhs,
+                          const llvm::APInt &rhs, bool isShiftLeft) {
+    return createShift(loc, lhs, getConstAPInt(loc, lhs.getType(), rhs),
+                       isShiftLeft);
+  }
+
+  mlir::Value createShift(mlir::Location loc, mlir::Value lhs, unsigned bits,
+                          bool isShiftLeft) {
+    auto width = mlir::dyn_cast<cir::IntType>(lhs.getType()).getWidth();
+    auto shift = llvm::APInt(width, bits);
+    return createShift(loc, lhs, shift, isShiftLeft);
+  }
+
+  mlir::Value createShiftLeft(mlir::Location loc, mlir::Value lhs,
+                              unsigned bits) {
+    return createShift(loc, lhs, bits, true);
+  }
+
+  mlir::Value createShiftRight(mlir::Location loc, mlir::Value lhs,
+                               unsigned bits) {
+    return createShift(loc, lhs, bits, false);
+  }
+
+  mlir::Value createShiftLeft(mlir::Location loc, mlir::Value lhs,
+                              mlir::Value rhs) {
+    return createShift(loc, lhs, rhs, true);
+  }
+
+  mlir::Value createShiftRight(mlir::Location loc, mlir::Value lhs,
+                               mlir::Value rhs) {
+    return createShift(loc, lhs, rhs, false);
+  }
+
   //
   // Block handling helpers
   // ----------------------

clang/include/clang/CIR/Dialect/IR/CIROps.td

@@ -1183,9 +1183,11 @@ def BinOp : CIR_Op<"binop", [Pure,
 def ShiftOp : CIR_Op<"shift", [Pure]> {
   let summary = "Shift";
   let description = [{
-    Shift `left` or `right`, according to the first operand. Second operand is
-    the shift target and the third the amount. Second and the thrid operand are
-    integers.
+    The `cir.shift` operation performs a bitwise shift, either to the left or to
+    the right, based on the first operand. The second operand specifies the
+    value to be shifted, and the third operand determines the number of
+    positions by which the shift is applied. Both the second and third operands
+    are required to be integers.
 
     ```mlir
     %7 = cir.shift(left, %1 : !u64i, %4 : !s32i) -> !u64i
@@ -1245,8 +1247,6 @@ def SelectOp : CIR_Op<"select", [Pure,
       qualified(type($false_value))
     `)` `->` qualified(type($result)) attr-dict
   }];
-
-  let hasFolder = 1;
 }
 
 //===----------------------------------------------------------------------===//

clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp

@@ -1308,9 +1308,7 @@ mlir::Value ScalarExprEmitter::emitShl(const BinOpInfo &ops) {
            mlir::isa<cir::IntType>(ops.lhs.getType()))
     cgf.cgm.errorNYI("sanitizers");
 
-  return builder.create<cir::ShiftOp>(cgf.getLoc(ops.loc),
-                                      cgf.convertType(ops.fullType), ops.lhs,
-                                      ops.rhs, cgf.getBuilder().getUnitAttr());
+  return builder.createShiftLeft(cgf.getLoc(ops.loc), ops.lhs, ops.rhs);
 }
 
 mlir::Value ScalarExprEmitter::emitShr(const BinOpInfo &ops) {
@@ -1334,8 +1332,7 @@ mlir::Value ScalarExprEmitter::emitShr(const BinOpInfo &ops) {
 
   // Note that we don't need to distinguish unsigned treatment at this
   // point since it will be handled later by LLVM lowering.
-  return builder.create<cir::ShiftOp>(
-      cgf.getLoc(ops.loc), cgf.convertType(ops.fullType), ops.lhs, ops.rhs);
+  return builder.createShiftRight(cgf.getLoc(ops.loc), ops.lhs, ops.rhs);
 }
 
 mlir::Value ScalarExprEmitter::emitAnd(const BinOpInfo &ops) {

clang/lib/CIR/Dialect/IR/CIRDialect.cpp

@@ -1049,28 +1049,6 @@ LogicalResult cir::ShiftOp::verify() {
   return mlir::success();
 }
 
-//===----------------------------------------------------------------------===//
-// SelectOp
-//===----------------------------------------------------------------------===//
-
-OpFoldResult cir::SelectOp::fold(FoldAdaptor adaptor) {
-  mlir::Attribute condition = adaptor.getCondition();
-  if (condition) {
-    bool conditionValue = mlir::cast<cir::BoolAttr>(condition).getValue();
-    return conditionValue ? getTrueValue() : getFalseValue();
-  }
-
-  // cir.select if %0 then x else x -> x
-  mlir::Attribute trueValue = adaptor.getTrueValue();
-  mlir::Attribute falseValue = adaptor.getFalseValue();
-  if (trueValue && trueValue == falseValue)
-    return trueValue;
-  if (getTrueValue() == getFalseValue())
-    return getTrueValue();
-
-  return {};
-}
-
 //===----------------------------------------------------------------------===//
 // UnaryOp
 //===----------------------------------------------------------------------===//

clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp

@@ -19,6 +19,7 @@
 #include "mlir/Dialect/DLTI/DLTI.h"
 #include "mlir/Dialect/Func/IR/FuncOps.h"
 #include "mlir/Dialect/LLVMIR/LLVMDialect.h"
+#include "mlir/IR/BuiltinAttributes.h"
 #include "mlir/IR/BuiltinDialect.h"
 #include "mlir/IR/BuiltinOps.h"
 #include "mlir/IR/Types.h"
@@ -28,6 +29,7 @@
 #include "mlir/Target/LLVMIR/Dialect/LLVMIR/LLVMToLLVMIRTranslation.h"
 #include "mlir/Target/LLVMIR/Export.h"
 #include "mlir/Transforms/DialectConversion.h"
+#include "clang/CIR/Dialect/IR/CIRAttrs.h"
 #include "clang/CIR/Dialect/IR/CIRDialect.h"
 #include "clang/CIR/Dialect/Passes.h"
 #include "clang/CIR/LoweringHelpers.h"
@@ -1318,8 +1320,7 @@ mlir::LogicalResult CIRToLLVMShiftOpLowering::matchAndRewrite(
   // be already be enforced at CIRGen.
   if (cirAmtTy)
     amt = getLLVMIntCast(rewriter, amt, mlir::cast<mlir::IntegerType>(llvmTy),
-                         !cirAmtTy.isSigned(), cirAmtTy.getWidth(),
-                         cirValTy.getWidth());
+                         true, cirAmtTy.getWidth(), cirValTy.getWidth());
 
   // Lower to the proper LLVM shift operation.
   if (op.getIsShiftleft()) {
@@ -1339,32 +1340,32 @@ mlir::LogicalResult CIRToLLVMShiftOpLowering::matchAndRewrite(
 mlir::LogicalResult CIRToLLVMSelectOpLowering::matchAndRewrite(
     cir::SelectOp op, OpAdaptor adaptor,
     mlir::ConversionPatternRewriter &rewriter) const {
-  auto getConstantBool = [](mlir::Value value) -> std::optional<bool> {
+  auto getConstantBool = [](mlir::Value value) -> cir::BoolAttr {
     auto definingOp =
         mlir::dyn_cast_if_present<cir::ConstantOp>(value.getDefiningOp());
     if (!definingOp)
-      return std::nullopt;
+      return {};
 
     auto constValue = mlir::dyn_cast<cir::BoolAttr>(definingOp.getValue());
     if (!constValue)
-      return std::nullopt;
+      return {};
 
-    return constValue.getValue();
+    return constValue;
   };
 
   // Two special cases in the LLVMIR codegen of select op:
   // - select %0, %1, false => and %0, %1
   // - select %0, true, %1 => or %0, %1
   if (mlir::isa<cir::BoolType>(op.getTrueValue().getType())) {
-    std::optional<bool> trueValue = getConstantBool(op.getTrueValue());
-    std::optional<bool> falseValue = getConstantBool(op.getFalseValue());
-    if (falseValue.has_value() && !*falseValue) {
+    cir::BoolAttr trueValue = getConstantBool(op.getTrueValue());
+    cir::BoolAttr falseValue = getConstantBool(op.getFalseValue());
+    if (falseValue && !falseValue.getValue()) {
       // select %0, %1, false => and %0, %1
       rewriter.replaceOpWithNewOp<mlir::LLVM::AndOp>(op, adaptor.getCondition(),
                                                      adaptor.getTrueValue());
       return mlir::success();
     }
-    if (trueValue.has_value() && *trueValue) {
+    if (trueValue && trueValue.getValue()) {
       // select %0, true, %1 => or %0, %1
       rewriter.replaceOpWithNewOp<mlir::LLVM::OrOp>(op, adaptor.getCondition(),
                                                     adaptor.getFalseValue());